-
Notifications
You must be signed in to change notification settings - Fork 68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cache: Add ReadThrough mode #386
Conversation
db00763
to
193908c
Compare
@DSilence The Though it also has to be said that these series of PRs (esp the cleanup ones I linked above) are the realisation of a lot of stuff you were pushing for at the time you introduced them (recall I didn't want to have a common cache impl until it REALLY hurt and then figure out the best design at that point) Spiked in #389 |
member x.Load(key, maxStaleness, compare, options, loadOrReload) = task { | ||
let loadOrReload maybeBaseState () = | ||
let act = System.Diagnostics.Activity.Current | ||
if act <> null then act.AddCacheHit(ValueOption.isSome maybeBaseState) |> ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW not certain whether there's value in separated metrics for incremental loads due to caching vs AllowStale roundtrips saved, or whether a cache hit is the main point - will be pondering
79fa120
to
154a6f0
Compare
Caching impl fixes Caching impl improvements
4c3c99f
to
9bdd317
Compare
Yes, sleeps in tests are bad news - PRs welcome ;)
This makes
decider.Query(..., load = Equinox.AllowStale (TimeSpan.FromSeconds 1)
opt into:This guarantees that the effects of processing reads against a stream whose core function is to manage the write side are limited
Cache
), ReadThrough reads can reuse that value, avoiding a retrieval roundtripOpen questions:
ShouldNo, people don't Dispose Caches IRL is the position for now...Cache
implementIDisposable
(it will trigger warnings in client mode asCache
construction will have to be called vianew Cache
)Should caching of state be made possible for MemoryStore too?Nah, not this time.AllowStale
becomeAnyCachedVersion
given the fact that less surprising semantics are now available? (should be considered alongside naming ofMaxStaleness
)MaxStaleness
toAllowStale
(means compiler errors for people leaning on the older behavior that's now entitledAnyCachedValue
, which I believe is a good tradeoff on balance)See also #388 and #380 for prep/tidy work on the way to this
Resolves #381