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 #5748

Closed
wants to merge 212 commits into from

Conversation

mgravell
Copy link
Member

@mgravell mgravell commented Dec 17, 2024

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
Microsoft Reviewers: Open in CodeFlow

joperezr and others added 30 commits October 2, 2024 21:58
Getting ready for 8.10 release

----
#### AI description  (iteration 1)
#### PR Classification
Release preparation

#### PR Summary
This pull request updates dependencies and configurations in preparation for the 8.10 release.
- `/eng/Version.Details.xml`: Updated various dependencies to newer versions.
- `/eng/Versions.props`: Synchronized dependency versions with the latest updates.
- `/azure-pipelines.yml`: Removed the `codecoverage` stage.
- `/eng/pipelines/templates/BuildAndTest.yml`: Added steps to set up private feed credentials for both Windows and non-Windows agents.
Getting ready for our 9.0-preview9 release

----
#### AI description  (iteration 1)
#### PR Classification
Dependency update

#### PR Summary
This pull request updates the project to use RC2 versions of ASP.NET Core and runtime dependencies.
- Updated dependency versions in `/eng/Version.Details.xml` and `/eng/Versions.props` to RC2.
- Removed code coverage stage from `azure-pipelines.yml`.
- Added setup for private feeds credentials in `/eng/pipelines/templates/BuildAndTest.yml`.
- Disabled NU1507 warning in `Directory.Build.props`.
- Modified `NuGet.config` to include `dotnet9-internal` feed and removed package source mappings.
…001.3 (dotnet#5468)

[main] Update dependencies from dotnet/arcade
…0241001.7 (dotnet#5469)

[main] Update dependencies from dotnet/aspnetcore
…003.2 (dotnet#5475)

[main] Update dependencies from dotnet/arcade
…0241007.7 (dotnet#5477)

Microsoft.AspNetCore.App.Ref , Microsoft.AspNetCore.App.Runtime.win-x64 , Microsoft.AspNetCore.Mvc.Testing , Microsoft.AspNetCore.TestHost , Microsoft.Extensions.Caching.SqlServer , Microsoft.Extensions.Caching.StackExchangeRedis , Microsoft.Extensions.Diagnostics.HealthChecks , Microsoft.Extensions.Http.Polly , Microsoft.Extensions.ObjectPool
 From Version 9.0.0-rtm.24501.7 -> To Version 9.0.0-rtm.24507.7

Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Steve Sanderson <SteveSandersonMS@users.noreply.github.com>
Co-authored-by: Eirik Tsarpalis <eirik.tsarpalis@gmail.com>
Co-authored-by: luisquintanilla <Luis.Quintanilla@microsoft.com>
…et#5485)

* Flip default on FunctionInvokingChatClient.ConcurrentInvocation

For better reliability, default ConcurrentInvocation to false, so that it doesn't introduce concurrency / parallelism where there wasn't any.

* Update src/Libraries/Microsoft.Extensions.AI/ChatCompletion/FunctionInvokingChatClient.cs

Co-authored-by: Igor Velikorossov <RussKie@users.noreply.github.com>

---------

Co-authored-by: Igor Velikorossov <RussKie@users.noreply.github.com>
…re] (dotnet#5492)

Given implementation details of the JSON source generator today, even with the converter applied to these properties, code is still being generated for Exception, leading to unsuppressable trimmer warnings.
* doc updates

* Update src/Libraries/Microsoft.Extensions.Caching.Hybrid/HybridCacheOptions.cs

---------

Co-authored-by: Igor Velikorossov <RussKie@users.noreply.github.com>
The underlying native schema support in OpenAI has specific requirements on valid schema names, as shown in the following exception when using either an array or any other generic type:

```
Unhandled exception. System.ClientModel.ClientResultException: HTTP 400 (invalid_request_error: invalid_value)
Parameter: response_format.json_schema.name

Invalid 'response_format.json_schema.name': string does not match pattern. Expected a string that matches the pattern '^[a-zA-Z0-9_-]+$'.
   at OpenAI.ClientPipelineExtensions.ProcessMessageAsync(ClientPipeline pipeline, PipelineMessage message, RequestOptions options)
```

This fix follows the approach used to sanitize function names, and sanitizes the schema name the same way.

Fixes dotnet#5501
Removed partial due error S2333 being shown otherwise.
…009.3 (dotnet#5506)

[main] Update dependencies from dotnet/arcade
RussKie and others added 3 commits December 17, 2024 15:31
* Bump code coverage
* Break builds only if coverage drops for 100% projects
* Update to public versions

* Update NuGet.config
@mgravell mgravell changed the base branch from dev to main December 20, 2024 16:48
@mgravell mgravell requested review from a team as code owners December 20, 2024 16:48
@mgravell mgravell changed the base branch from main to dev December 20, 2024 16:50
@sebastienros
Copy link
Member

I am not sure GetTimestamp() should be used to store the moment, I tried this code on Linux and Windows and got different results:

Console.WriteLine(TimeProvider.System.GetTimestamp());
Console.WriteLine(TimeProvider.System.GetUtcNow());
Console.WriteLine(System.Runtime.InteropServices.RuntimeInformation.OSDescription);
8226721287470
12/20/2024 5:27:32 PM +00:00
Microsoft Windows 10.0.17763
5640002254299340
12/20/2024 17:27:24 +00:00
Ubuntu 24.04.1 LTS

@mgravell
Copy link
Member Author

mgravell commented Dec 20, 2024

@sebastienros thanks, great catch! I incorrectly assumed that this was a defined value like epoch ticks or something. Apparently not. I'll rework that into something reliable.

@mgravell
Copy link
Member Author

There is TimestampFrequency, but I will change to use GetUtcNow() instead.

@sebastienros
Copy link
Member

Use ticks (but same normalization issue) or a sortable numeric format (yyyymmddhhmmssxxxx), or use an EPOCH so 0 starts at 1/1/2025 ;) then numbers are much smaller.

@mgravell
Copy link
Member Author

I considered that - if you think it won't be contentious, sure I can use an atypical epoch. At stackoverflow, we used "stack seconds" in a few key places :) however, the fixed size is highly desirable, and I'd rather not get into 32-bit pain. Honestly, I think it is better to just pay for 64 and not worry about it.

@jodydonetti
Copy link
Contributor

FWIW DateTimeOffset.Ticks/UtcTicks seems to be consistent cross platform.

@mgravell
Copy link
Member Author

mgravell commented Jan 7, 2025

rebase went really, really freaky; trying to resolve, but it might be easier to re-branch and copy just the HC bits

@mgravell
Copy link
Member Author

mgravell commented Jan 8, 2025

moving to #5781 due to rebase fail

re the GetTimestamp() usage; upon review, the only place I could find that was in the tests where it was only used for "is it the same now?", but: that has now been removed (/cc @sebastienros )

@mgravell mgravell closed this Jan 8, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Feb 8, 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.