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

HybridCache : implement the tag expiration feature #5781

Closed
wants to merge 6 commits into from

Conversation

mgravell
Copy link
Member

@mgravell mgravell commented Jan 8, 2025

(note: this is a re-do of #5748 which went "a bit wrong" when rebasing)

Implement tag expiration in HybridCache

The HybridCache system has a "tags" feature; rather than building a bespoke per-backend secondary index (like how the redis OutputCache backend works), here we move everything into the core library:

  • in the DefaultHybridCache, we maintain a map of tags (string, note * means everything) to expirations, and we track the creation time against each CacheItem
  • when fetching items from L1 (in-proc) cache, we check the item's creation data against the cache's wildcard and per-tag expirations; if the creation date is earlier than any of these: it is treated as disallowed
  • to allow persistence of invalidation between sessions when a L2 (backend cache) is in play, we additionally store the timeout data via additional simple values in the cache
  • we also pre-fetch any missing tag data from L2 as-needed, and enforce the previous expirations
  • to facilitate this, and allow reliability that the data we are parsing is the data requested: the data stored in L2 is now embedded inside a payload that includes the creation timestamp, duration, key, tags, etc as well as the payload
  • in the rare event that the L2 data contains additional tags to those advertised by the caller: these additional tags are also fetched and enforced

Because the tag fetches are async, the data in the lookup is not (timestamp), it is Task<(timestamp)>

Outstanding:

  • additional logging
    • add log when L2 data is rejected for (reason)
    • add metric of the number of tag expirations being tracked in L1
  • resolve netfx test failure (actually: niche timing problem in tag invalidation)
  • address option member inheritance (or not - per @jodydonetti comments here)
Microsoft Reviewers: Open in CodeFlow

@mgravell
Copy link
Member Author

mgravell commented Jan 8, 2025

inviting @sebastienros and @jodydonetti from #5748

@jodydonetti
Copy link
Contributor

jodydonetti commented Jan 8, 2025

Hi Marc, not strictly related to tagging, but I'm looking at some code change in the PR and I spotted this xml comment regarding HybridCacheEntryOptions:

If options are specified at the individual call level, the non-null values are merged
(with the per-call options being used in preference to the global options). If no value is
specified for a given option (globally or per-call), the implementation can choose a reasonable default.

When we touched on this subject some time ago I remember we agreed on the fact that inheritance (from per-call up to the DefaultEntryOptions) would have to be done on an all-or-nothing approach, and not on a per-single-option approach.
What I mean is that "if null is passed as an HybridCacheEntryOptions param" than it would fall back to the DefaultEntryOptions, but single options inside that object (like Expiration, LocalCacheExpiration) should not have inheritance of values.

All of this was because of the impossibility to express "undefined" or, to better say it, the impossibility to differentiate between null meaning "I'm saying null because I want a local fallback" (for example from LocalCacheExpiration to Expiration) and null meaning "I'm saying null because I want a global fallback" (for example to the same option in the DefaultEntryOptions").
A practical example of a problem this may create: in the DefaultEntryOptions I set Expiration to 5min and LocalCacheExpiration to 10s to refresh L1 more frequently. Then in a certain call I pass a specific HybridCacheEntryOptions instance where I set Expiration to 1h and leave LocalCacheExpiration to null, thinking it will be the same as Expiration, when in reality it will become 10s. Because of this, there will be for me no way to keep the 2 options in-sync at every call, unless I keep specifying the same value for both Expiration and LocalCacheExpiration at every call.

Can you clarify what is the current rationale here?

I'm asking for 2 reasons: first I'm interested in general, and second because as you know I'm creating a 3rd party implementation, and I'd like te observable external behaviour to be the same for both implementations.

Thanks!

ps: if you like I can open a different issue for this, just let me know.
pps: will post more about the tagging implementation as soon as I'll finish reading the massive code changes.

@mgravell
Copy link
Member Author

mgravell commented Jan 8, 2025

@jodydonetti thanks; added checklist item

@dotnet-comment-bot
Copy link
Collaborator

‼️ Found issues ‼️

Project Coverage Type Expected Actual
Microsoft.Extensions.Caching.Hybrid Line 86 83.29 🔻
Microsoft.Extensions.Caching.Hybrid Branch 86 83.51 🔻

🎉 Good job! The coverage increased 🎉
Update MinCodeCoverage in the project files.

Project Expected Actual
Microsoft.Extensions.Diagnostics.Probes 70 76

Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=910739&view=codecoverage-tab

@dotnet-comment-bot
Copy link
Collaborator

‼️ Found issues ‼️

Project Coverage Type Expected Actual
Microsoft.Extensions.Caching.Hybrid Line 86 84.26 🔻
Microsoft.Extensions.Caching.Hybrid Branch 86 84.41 🔻

🎉 Good job! The coverage increased 🎉
Update MinCodeCoverage in the project files.

Project Expected Actual
Microsoft.Extensions.Diagnostics.Probes 70 76

Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=911896&view=codecoverage-tab

@mgravell mgravell closed this Jan 9, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Feb 9, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants