Split redis output-cache pieces from Microsoft.Extensions.Caching.StackExchange.Redis into Microsoft.AspNetCore.OutputCaching.StackExchangeRedis #50457

mgravell · 2023-08-31T16:51:04Z

Split redis output-cache pieces from Microsoft.Extensions.Caching.StackExchange.Redis into Microsoft.AspNetCore.OutputCaching.StackExchangeRedis

Description

Split SE.Redis OutputCache implementation into new package, Microsoft.AspNetCore.OutputCaching.StackExchangeRedis (it was causing an undesirable framework ref)

remove the output cache functionality and ref from Microsoft.Extensions.Caching.StackExchange.Redis
- (simplified API [un]shipped.txt back to single; no change except no more unshipped)
moved functionality to new package (and update eng files accordingly)
new namespace etc; will need API review
in particular, RedisCacheOptions was shared between both APIs; now RedisOutputCacheOptions

Related: #50402

….AspNetCore.OutputCaching.StackExchangeRedis (it was causing an undesirable framework ref) - remove the output cache functionality and ref from Microsoft.Extensions.Caching.StackExchange.Redis - (simplified API [un]shipped.txt back to single; no change except no more unshipped) - moved functionality to new package (and update eng files accordingly) - new namespace etc; will need API review - in particular, RedisCacheOptions was shared between both APIs; now RedisOutputCacheOptions

...tCaching.StackExchangeRedis/src/Microsoft.AspNetCore.OutputCaching.StackExchangeRedis.csproj

src/Caching/StackExchangeRedis/src/Microsoft.Extensions.Caching.StackExchangeRedis.csproj

mgravell · 2023-09-01T15:20:11Z

oof, it really isn't happy about me monkeying with the shipped/unshipped files; I might have to revert that to prior state for now

does that look better @wtgodbe ?

wtgodbe

I think we're good now - .Shipped.txt files should never be subtracted from, so that looks to be in good shape. CI will complain that the files were changed at all in a release/* branch, but in this case it's fine since it's RC2, so I can just force-merge this once it's good to go. Do you want anyone else to review it?

BrennanConroy · 2023-09-03T23:19:01Z

Send an email to API review.

Tratcher · 2023-09-05T15:47:38Z

src/Middleware/Microsoft.AspNetCore.OutputCaching.StackExchangeRedis/src/RedisCacheOptions.cs

+/// <summary>
+/// Configuration options for Redis based output cache.
+/// </summary>
+public class RedisOutputCacheOptions : IOptions<RedisOutputCacheOptions>


Is there a way we can depend on the existing StackExchangeRedis package and share/derive from its options type and otherwise reduce duplication?

In theory, yes. However, it seems an odd choice to take a package dependency just for a config definition.

If I wanted a Distributed Cache and an Output Cache in my app that both talked to the same redis server, do I need to now configure them both the same way?

I wonder if we could put ConfigurationOptions as the thing that is an IOptions that I can configure. That way apps can configure redis distributed caches and redis output caches the same way and they compose together.

It seems like a maintenance and config issue. E.g. having to add, implement, and configure options in both places.

I don't quite follow your suggestion @eerhardt. It would be nice to configure the two together somehow.

I don't quite follow your suggestion

My suggestion is to use the StackExchange.Redis.ConfigurationOptions type as the "Options" object. That is what is configured in DI instead of these separate RedisOutputCacheOptions and RedisCacheOptions objects which have properties of type StackExchange.Redis.ConfigurationOptions.

One issue with IOptions is that they don't compose together well. You can't have have 2 IOptions objects that refer to the same instance without manually writing code to wire them together.

@Tratcher I 100% agree on the configure together thing; an option there is to use the option that provides the multiplexer directly, although that then doesn't have quite the same capability re recreating the connection; I suggest we tackle this in net9, though

eerhardt · 2023-09-05T23:08:43Z

src/Middleware/Microsoft.AspNetCore.OutputCaching.StackExchangeRedis/src/RedisCacheOptions.cs

+/// <summary>
+/// Configuration options for Redis based output cache.
+/// </summary>
+public class RedisOutputCacheOptions : IOptions<RedisOutputCacheOptions>


Suggested change

public class RedisOutputCacheOptions : IOptions<RedisOutputCacheOptions>

public class RedisOutputCacheOptions

This class doesn't need to implement IOptions. I know the existing distributed cache implements it, but I believe that is a mistake (that we can't fix because it is a shipped API).

Option classes are POCOs, and when someone consumes them in DI, that's when they use IOptions<XXX>.

For example:

aspnetcore/src/Servers/Kestrel/Core/src/Internal/KestrelServerImpl.cs

Lines 36 to 37 in 00d0038

public KestrelServerImpl(

IOptions<KestrelServerOptions> options,

aspnetcore/src/Servers/Kestrel/Core/src/KestrelServerOptions.cs

Lines 27 to 28 in 00d0038

public class KestrelServerOptions

{

KestrelServerOptions doesn't implement IOptions itself.

We did it in a few types like MemoryCache/Options where people would actually new up the options and class themselves. That's likely not the case here.

@Tratcher so is that a "leave alone" or a "remove the IOptions"? happy with either

Remove it. You don't new up a RedisOutputCache instance directly do you?

no; nuking, ta

ah, it was probably there for the test; I switched them to use DI properly instead - sound like a plan? 7c915d4#diff-9b6a4cced77cfd298a082285b4f112b7e5a97059a80e5291df832b037e0e996a

src/Middleware/Microsoft.AspNetCore.OutputCaching.StackExchangeRedis/src/RedisCacheOptions.cs

… the tests)

mgravell · 2023-09-07T20:07:18Z

Now just needs someone like @wtgodbe or @adityamandaleeka to merge?

wtgodbe · 2023-09-11T16:42:51Z

Re-running because of the helix failure

mgravell · 2023-09-12T10:10:33Z

@wtgodbe is there anything magic we need to do to ensure the new OOB lib gets nugetized, or should that happen automatically?

ghost · 2023-09-12T10:10:36Z

Hi @mgravell. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

eerhardt · 2023-09-12T15:18:50Z

@wtgodbe is there anything magic we need to do to ensure the new OOB lib gets nugetized, or should that happen automatically?

It should happen automatically because of these two lines:

aspnetcore/src/Middleware/Microsoft.AspNetCore.OutputCaching.StackExchangeRedis/src/Microsoft.AspNetCore.OutputCaching.StackExchangeRedis.csproj

Lines 8 to 9 in 95f0e04

    
           <IsPackable>true</IsPackable> 
        
           <IsShipping>true</IsShipping>

You can see the new package at https://dev.azure.com/dnceng/public/_artifacts/feed/dotnet8/NuGet/Microsoft.AspNetCore.OutputCaching.StackExchangeRedis/overview

ghost · 2023-09-12T15:18:54Z

Hi @eerhardt. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

ghost added the area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares label Aug 31, 2023

wtgodbe reviewed Aug 31, 2023

View reviewed changes

...tCaching.StackExchangeRedis/src/Microsoft.AspNetCore.OutputCaching.StackExchangeRedis.csproj Outdated Show resolved Hide resolved

wtgodbe reviewed Aug 31, 2023

View reviewed changes

src/Caching/StackExchangeRedis/src/Microsoft.Extensions.Caching.StackExchangeRedis.csproj Outdated Show resolved Hide resolved

mgravell added 3 commits August 31, 2023 18:36

update tags

3e7ca11

add a startvs.cmd

a9018f0

remove ExcludeFromSourceBuild

8c57329

mgravell marked this pull request as ready for review September 1, 2023 15:14

mgravell requested review from Tratcher, BrennanConroy, a team, captainsafia and halter73 as code owners September 1, 2023 15:14

mgravell added 2 commits September 1, 2023 16:27

revert shipped/unshipped to make CI happy

66b48cf

keep public API file position to minimize PR size

6ba6f96

mgravell requested a review from wtgodbe September 1, 2023 15:31

wtgodbe approved these changes Sep 1, 2023

View reviewed changes

mgravell mentioned this pull request Sep 4, 2023

Add IOutputCacheBufferWriterStore #48854

Closed

Tratcher reviewed Sep 5, 2023

View reviewed changes

Tratcher approved these changes Sep 5, 2023

View reviewed changes

eerhardt reviewed Sep 5, 2023

View reviewed changes

JamesNK reviewed Sep 6, 2023

View reviewed changes

src/Middleware/Microsoft.AspNetCore.OutputCaching.StackExchangeRedis/src/RedisCacheOptions.cs Outdated Show resolved Hide resolved

seal RedisOutputCacheOptions

23eb427

mgravell requested a review from JamesNK September 7, 2023 13:03

mgravell added 3 commits September 7, 2023 14:09

file named incorrectly

dfe803e

unnecessary using directive

b92051a

remove IOptions<T> from RedisOutputCacheOptions (required using DI in…

7c915d4

… the tests)

wtgodbe merged commit c65d9a1 into release/8.0 Sep 11, 2023

wtgodbe deleted the marc/oc_redis_lib_split_8_0 branch September 11, 2023 20:09

ghost added this to the 8.0-rc2 milestone Sep 11, 2023

ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Sep 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split redis output-cache pieces from Microsoft.Extensions.Caching.StackExchange.Redis into Microsoft.AspNetCore.OutputCaching.StackExchangeRedis #50457

Split redis output-cache pieces from Microsoft.Extensions.Caching.StackExchange.Redis into Microsoft.AspNetCore.OutputCaching.StackExchangeRedis #50457

mgravell commented Aug 31, 2023

mgravell commented Sep 1, 2023 •

edited

Loading

wtgodbe left a comment

BrennanConroy commented Sep 3, 2023

Tratcher Sep 5, 2023

mgravell Sep 5, 2023

eerhardt Sep 5, 2023

Tratcher Sep 5, 2023

eerhardt Sep 6, 2023

mgravell Sep 7, 2023

eerhardt Sep 5, 2023

Tratcher Sep 5, 2023

mgravell Sep 7, 2023

Tratcher Sep 7, 2023

mgravell Sep 7, 2023

mgravell Sep 7, 2023

Tratcher Sep 7, 2023

mgravell commented Sep 7, 2023

wtgodbe commented Sep 11, 2023

mgravell commented Sep 12, 2023

ghost commented Sep 12, 2023

eerhardt commented Sep 12, 2023 •

edited

Loading

ghost commented Sep 12, 2023

	public class RedisOutputCacheOptions : IOptions<RedisOutputCacheOptions>
	public class RedisOutputCacheOptions

	public KestrelServerImpl(
	IOptions<KestrelServerOptions> options,

Split redis output-cache pieces from Microsoft.Extensions.Caching.StackExchange.Redis into Microsoft.AspNetCore.OutputCaching.StackExchangeRedis #50457

Split redis output-cache pieces from Microsoft.Extensions.Caching.StackExchange.Redis into Microsoft.AspNetCore.OutputCaching.StackExchangeRedis #50457

Conversation

mgravell commented Aug 31, 2023

Split redis output-cache pieces from Microsoft.Extensions.Caching.StackExchange.Redis into Microsoft.AspNetCore.OutputCaching.StackExchangeRedis

Description

mgravell commented Sep 1, 2023 • edited Loading

wtgodbe left a comment

Choose a reason for hiding this comment

BrennanConroy commented Sep 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgravell commented Sep 7, 2023

wtgodbe commented Sep 11, 2023

mgravell commented Sep 12, 2023

ghost commented Sep 12, 2023

eerhardt commented Sep 12, 2023 • edited Loading

ghost commented Sep 12, 2023

mgravell commented Sep 1, 2023 •

edited

Loading

eerhardt commented Sep 12, 2023 •

edited

Loading