-
Notifications
You must be signed in to change notification settings - Fork 10.3k
RedisCacheOptions to Manually Set ConnectionMultiplexer #31879
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
Conversation
/// <summary> | ||
/// Gets or sets a delegate to create the ConnectionMultiplexer instance. | ||
/// </summary> | ||
public Func<IConnectionMultiplexer> ConnectionMultiplexerFactory { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be nullable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public Func<IConnectionMultiplexer> ConnectionMultiplexerFactory { get; set; } | |
[DisallowNull] | |
public Func<Task<IConnectionMultiplexer>>? ConnectionMultiplexerFactory { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is making this a Task
return type buying us exactly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davidfowl mentioned creating the connection involved I/O. This was meant to reflect that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Func will likely include an async call e.g. ConnectionMultiplexer.ConnectAsync
and this way, the Func can be awaited in other components to get the same connection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JunTaoLuo and @pranavkm I am having some issues with this. Nullable annotations does not appear to be enabled in this class yet:
/Users/dev-spv/aspnetcore/src/Caching/StackExchangeRedis/src/RedisCacheOptions.cs(32,65): error CS8632: The annotation for nullable reference types should only be used in code within a '#nullable' annotations context. [/Users/dev-spv/aspnetcore/src/Caching/StackExchangeRedis/src/Microsoft.Extensions.Caching.StackExchangeRedis.csproj]
I am willing to add these annotations if desired, but I had a ton of issues trying to satisfy the Roslyn Unshipped API file changes with VSCode. Specifically it would not recognize the get as public even though it recognized it was part of the API. Below is what I could not get working in the unshipped text file even when I did have #nullable enabled
at the top of the class:
Microsoft.Extensions.Caching.StackExchangeRedis.RedisCacheOptions.ConnectionMultiplexerFactory.get -> System.Func<System.Threading.Tasks.Task<StackExchange.Redis.IConnectionMultiplexer>>? Microsoft.Extensions.Caching.StackExchangeRedis.RedisCacheOptions.ConnectionMultiplexerFactory.set -> void
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's continue the conversation at #32266
PR Title
RedisCacheOptions to Manually Set ConnectionMultiplexer
PR Description
Adding a Factory Func to allow manual setting of the
ConnectionMultiplexer
for Redis.Implemented for both Async and Sync method paths.
Addresses #28379