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

Migrate HybridCache from aspnetcore to extensions #5391

Merged
merged 10 commits into from
Sep 11, 2024

Conversation

mgravell
Copy link
Member

@mgravell mgravell commented Aug 28, 2024

Plan agreed in "tactics" to migrate HybridCache from dotnet/aspnetcore to dotnet/extensions.

This is a direct snapshot of the code from this point in dotnet/aspnetcore, with minor modifications to the library code:

  1. to satisfy local repo style rules (member order, comment location, spacing, etc)
  2. work with different build setup (csproj delta)

Minor global changes:

  1. support netstandard2.1 on some legacy shim includes (the previous preview builds have included ns2.0 and ns2.1 targets)
  2. add some additional required packages (for testing)
Microsoft Reviewers: Open in CodeFlow

Reasons for move:

  1. additional work required before GA (question: should we mark this <IsPackable>false</IsPackable> for now?)
  2. to utilize higher release cadence and additional change flexibility for OOB NuGet packages

@mgravell mgravell changed the base branch from main to dev August 28, 2024 13:34
@mgravell
Copy link
Member Author

mgravell commented Aug 28, 2024

remaining Correctness WarningCheck fail is about adding [Experimental]; happy to take guidance here on the best approach; I have a spike locally with [Experimental] using token EXTEXP0018 (IIRC), but not sure how we add the aka.ms link doc.

@mgravell

This comment was marked as resolved.

@RussKie

This comment was marked as resolved.

@RussKie
Copy link
Member

RussKie commented Aug 29, 2024

remaining Correctness WarningCheck fail is about adding [Experimental]; happy to take guidance here on the best approach; I have a spike locally with [Experimental] using token EXTEXP0018 (IIRC), but not sure how we add the aka.ms link doc.

Yes, let's mark it as [Experimental].
We'll need to add an entry in to https://github.com/dotnet/extensions/blob/main/docs/list-of-diagnostics.md. I can show you how to add a redirection link when we merge this.

Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

I started the review before I realised this changeset moves implementations from dotnet/aspnetcore. I left a number of style-related comments, as I'm guessing dotnet/aspnetcore follows a different style to us.

Besides that there are few other comments and questions. I find the use of ! operator and reliance one Debug.Assert somewhat worrisome, it does look those cases could result either in NREs or incorrect behaviours in release builds.

Overall, it would be good to understand the target state of this code move (since I'm not attending the Tactics) - will dotnet/aspnetcore version be deprecated, and dotnet/aspnetcore will instead depend on our artifacts and implementations? What would this mean for consumers? Etc.

@RussKie RussKie requested a review from joperezr August 29, 2024 03:39
@RussKie RussKie added the waiting-author-feedback 📭 The author of this issue needs to respond in order for us to continue investigating this issue. label Aug 29, 2024
@mgravell
Copy link
Member Author

Yes, let's mark it as [Experimental].
We'll need to add an entry in to https://github.com/dotnet/extensions/blob/main/docs/list-of-diagnostics.md. I can show you how to add a redirection link when we merge this.

Discussed this with Jose separately; this API has already been shipped non-[Experimental] in multiple net9 previews; might be confusing to add it now. That said, I'm happy with any outcome - this is not a hill I need to die on, as long as it is appropriately considered ;p

@dotnet-policy-service dotnet-policy-service bot removed the waiting-author-feedback 📭 The author of this issue needs to respond in order for us to continue investigating this issue. label Aug 29, 2024
@mgravell
Copy link
Member Author

@RussKie thanks for the detailed feedback; commit with tweaks pushed

@mgravell
Copy link
Member Author

Besides that there are few other comments and questions. I find the use of ! operator and reliance one Debug.Assert somewhat worrisome, it does look those cases could result either in NREs or incorrect behaviours in release builds.

The assertions here are for things that we already absolutely positively already believe to be true and pre-validated by separate logic, but we just want to be paranoid in debug, because of paranoia.

Overall, it would be good to understand the target state of this code move (since I'm not attending the Tactics) - will dotnet/aspnetcore version be deprecated, and dotnet/aspnetcore will instead depend on our artifacts and implementations? What would this mean for consumers? Etc.

Yes, the plan is to relocate HybridCache from dotnet/aspnetcore to dotnet/extensions (but that doesn't mean it becomes "your problem" - it is still 100% my fault on any ticket items here). Basically, this is an OOB NuGet package that has absolutely no need to be tied to dotnet/aspnetcore other than habit. We actively want to make use of the different build train here; however, there should be no change to consumers (other than we have more flexibility to evolve and iterate out-of-step of runtime/aspnetcore).

@RussKie
Copy link
Member

RussKie commented Aug 29, 2024

Yes, the plan is to relocate HybridCache from dotnet/aspnetcore to dotnet/extensions (but that doesn't mean it becomes "your problem" - it is still 100% my fault on any ticket items here). Basically, this is an OOB NuGet package that has absolutely no need to be tied to dotnet/aspnetcore other than habit. We actively want to make use of the different build train here; however, there should be no change to consumers (other than we have more flexibility to evolve and iterate out-of-step of runtime/aspnetcore).

Sounds good, the more - the merrier :)

@RussKie
Copy link
Member

RussKie commented Aug 29, 2024

Yes, let's mark it as [Experimental].
We'll need to add an entry in to main/docs/list-of-diagnostics.md. I can show you how to add a redirection link when we merge this.

Discussed this with Jose separately; this API has already been shipped non-[Experimental] in multiple net9 previews; might be confusing to add it now. That said, I'm happy with any outcome - this is not a hill I need to die on, as long as it is appropriately considered ;p

I wasn't in the loop.
But since this is an OOB package, and we're just changing the repo and leaving the implementation as-is (bar styling changes), then, it makes sense to ship it as-is too (i.e., non-experimental). In that case, we'll need to set the Stage=normal (which has few additional implications - like certain test coverage levels). I'll have to brush up on that.

@RussKie
Copy link
Member

RussKie commented Sep 2, 2024

Yes, let's mark it as [Experimental].
We'll need to add an entry in to main/docs/list-of-diagnostics.md. I can show you how to add a redirection link when we merge this.

Discussed this with Jose separately; this API has already been shipped non-[Experimental] in multiple net9 previews; might be confusing to add it now. That said, I'm happy with any outcome - this is not a hill I need to die on, as long as it is appropriately considered ;p

In this repo we only have two states - "dev" and "normal" or (in a consumer terms) - "experimental" and "prod-ready". All our packages except for the following three are "prod-ready", these three are "experimental":

  • Microsoft.Extensions.Diagnostics.Probes
  • Microsoft.Extensions.Hosting.Testing
  • Microsoft.Extensions.Options.Contextual

During the "9.0-previewX" releases there are not visible branding difference, however, at and after the official release the "prod-ready" packages have stable branding, e.g.: "Microsoft.Extensions.Compliance.Testing.8.8.0.nupkg"; and the "experimental" packages have not, e.g.: "Microsoft.Extensions.Diagnostics.Probes.8.8.0-rtm.24412.9.nupkg".

The "experimental" packages are also decorated with the ExperimentalAttribute at the assembly level with the assigned warning code.
E.g.:

<!-- In order to get the right package versions for projects that shouldn't stabilize, we need to set this property before
importing the root level Directory.Build.props file. This property should be kept in here, as opposed to moving it to
the project itself. -->
<PropertyGroup>
<Stage>dev</Stage>
<StageDevDiagnosticId>EXTEXP0017</StageDevDiagnosticId>
</PropertyGroup>

So... if we're not expecting any further API changes in this package, then we can mark the package as "prod-ready", i.e., set <Stage>normal</Stage>. Otherwise, we need to mark the package as <Stage>dev</Stage>

We also need to do the following:

  • ensure the test coverage is above 75% (i.e., <MinCodeCoverage>XX</MinCodeCoverage>, where XX is an integer in [75, 100]).
  • since we're currently not running any mutation tests, MinMutationScore can be set to anything above 50 (if necessary),
  • run ApiChief to generate the baseline
    dotnet run --project .\eng\Tools\ApiChief\ApiChief.csproj .\artifacts\bin\Microsoft.Extensions.Caching.Hybrid\Debug\net9.0\Microsoft.Extensions.Caching.Hybrid.dll emit baseline > .\src\Libraries\Microsoft.Extensions.Caching.Hybrid\Microsoft.Extensions.Caching.Hybrid.json

/cc: @joperezr

this is a direct snapshot from dotnet/aspnetcore, with modifications to:

1. satisfy local repo style rules
2. work with different build setup

Minor global changes:

1. support netstandard2.1 on some legacy shim includes
2. add some additional required packages (for testing)
re .dic changes: adds "memoize[s|d]", "mibi", "canceled", and a few similar; happy to revert if disagreeable
@mgravell
Copy link
Member Author

mgravell commented Sep 2, 2024

(rebased against dev)

eng/Versions.props Outdated Show resolved Hide resolved
@@ -0,0 +1 @@
Full `HybridCache` documentation is [here](https://learn.microsoft.com/aspnet/core/performance/caching/hybrid).
Copy link
Member

Choose a reason for hiding this comment

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

We do try to enforce that all packages produced out of this repo have a README that more or less follows the same format (brief description of the package intent, Instructions for installing the package, Usage Examples). You can see an example of this here: https://github.com/dotnet/extensions/blob/main/src/Libraries/Microsoft.Extensions.Diagnostics.HealthChecks.Common/README.md. They don't have to be very exahustive, but it would be good to have at least a very basic README here with at least some of those details as opposed to just pointing to some other learn page. The main idea of this README is: "What would a consumer NEED TO KNOW in the first 5 minutes of consuming the package?" That means it is totally fine to still have more detailed documentation elsewhere and we specifically don't want this README to be big, but it would be good to follow pattern if possible.

Copy link
Member

@joperezr joperezr left a comment

Choose a reason for hiding this comment

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

Left a couple of minor comments, but other than filling in the README and onboarding to dependency flow, this PR looks good to me. Thanks for working on it @mgravell and thanks for taking a look @RussKie

@RussKie
Copy link
Member

RussKie commented Sep 4, 2024

To confirm - is the decision to ship this package and the API within as "stable/prod-ready"?

@joperezr
Copy link
Member

joperezr commented Sep 4, 2024

To confirm - is the decision to ship this package and the API within as "stable/prod-ready"?

If the intention is whether or not the APIs should be experimental, I don't think it is needed, I would leave that up to @mgravell's discretion. The intent of that analyzer requiring you to mark the APIs as experimental the first time you introduce them is to make sure that you at least release them once and let customers use it before you mark them as stable. In this library's case, it's API has already been shipped as preview multiple times and people has shared feedback, so I don't think that is a concern. Also, this is in the dev branch which means that even if this gets merged now, it would still only ship as preview.

@RussKie RussKie added the waiting-author-feedback 📭 The author of this issue needs to respond in order for us to continue investigating this issue. label Sep 9, 2024
@dotnet-policy-service dotnet-policy-service bot removed the waiting-author-feedback 📭 The author of this issue needs to respond in order for us to continue investigating this issue. label Sep 11, 2024
- reorder .ctor first
- add baseline API (instead of suppressing)
@mgravell
Copy link
Member Author

Suggestions integrated; code remains as per vanilla "relocate from aspnetcore main"

@mgravell
Copy link
Member Author

@RussKie @joperezr am I good to merge? (I have permissions - I just don't want to stomp if that's not your preferred policy)

@RussKie
Copy link
Member

RussKie commented Sep 11, 2024

@RussKie @joperezr am I good to merge? (I have permissions - I just don't want to stomp if that's not your preferred policy)

Yes (squash-merge).

But since I'm already here, I'll save you the troubles and merge it now :)

@RussKie RussKie merged commit 579b89e into dotnet:dev Sep 11, 2024
6 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Oct 13, 2024
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