Skip to content
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

Merged
merged 22 commits into from
Jun 5, 2023
Merged

Cache: Add ReadThrough mode #386

merged 22 commits into from
Jun 5, 2023

Conversation

bartelink
Copy link
Collaborator

@bartelink bartelink commented May 15, 2023

This makes decider.Query(..., load = Equinox.AllowStale (TimeSpan.FromSeconds 1) opt into:

  1. have all reads where the cache entry is stale or missing share a single roundtrip to load the state from the store
  2. cache the value for the specified time (from the start of the retrieval, not when it arrived)

This guarantees that the effects of processing reads against a stream whose core function is to manage the write side are limited

  • the readers within a given process will self-limit themselves to maximum one roundtrip per specified period
  • if a writer updates in the interim (using the same Cache), ReadThrough reads can reuse that value, avoiding a retrieval roundtrip

Open questions:

  • Should Cache implement IDisposable (it will trigger warnings in client mode as Cache construction will have to be called via new Cache) No, people don't Dispose Caches IRL is the position for now...
  • Should caching of state be made possible for MemoryStore too? Nah, not this time.
  • Should AllowStale become AnyCachedVersion given the fact that less surprising semantics are now available? (should be considered alongside naming of MaxStaleness)
  • Rename MaxStaleness to AllowStale (means compiler errors for people leaning on the older behavior that's now entitled AnyCachedValue, 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

@bartelink bartelink changed the title Separate IReloadable ReadThrough mode May 15, 2023
@bartelink bartelink changed the base branch from master to cache-cleanup May 18, 2023 09:29
@bartelink bartelink force-pushed the bounded-staleness branch 4 times, most recently from db00763 to 193908c Compare May 18, 2023 10:18
@bartelink
Copy link
Collaborator Author

bartelink commented May 18, 2023

@DSilence The ICache interface changes significantly, and is obviously harder to implement now, as it does more.
I don't hate it, but have to say I am tempted to remove ICache as I'm not convinced about there being meaningful implementations out there

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

@bartelink bartelink marked this pull request as ready for review May 18, 2023 10:48
@bartelink bartelink changed the title ReadThrough mode Cache: Add ReadThrough mode May 18, 2023
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏

Copy link
Collaborator Author

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

@bartelink bartelink force-pushed the bounded-staleness branch 8 times, most recently from 79fa120 to 154a6f0 Compare May 25, 2023 09:28
Base automatically changed from cache-cleanup to master June 3, 2023 00:06
@bartelink bartelink merged commit b33b08f into master Jun 5, 2023
@bartelink bartelink deleted the bounded-staleness branch June 5, 2023 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: ReadThrough mode
2 participants