Configuration: An idea on separation #1987
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is just an idea pitch to see if the direction even works. The intent is for a consumer or wrapper library to specify a default options provider (or extend the built-in ones). For example Azure could override IsMatch and maintain its own domain list, as could AWS, etc. Problems with this: - Modifying the `KnownProviders` list on `DefaultOptionsProvider` is awkward, but ConfigurationOptions can also get a `.Defaults` set directly, or...other ideas? Maybe `GetForEndpoints` should get on `ConfigurationOptions` as a `public static` instead? - Maintenance events are not fully decoupled: they're calling `internal` bits on `ConnectionMultiplexer`, so we'd either need to expose those in some way (e.g. being able to trigger a reconfigure) or...something else. - The client name including Azure role bits are still on `DefaultOptionsProvider`, so that client names showing role instances are still global of where the client runs, and not narrowed down to only when connecting to Azure endpoints. Overall I think the `ConfigurationOptions` bits are net cleaner but maintenance events aren't capable of being moved out. If we exposed `ReconfigureAsync` via `ServerMaintenanceEvent` base in some way, it could be extensible in that way without over-exposing the deep internals. Needs eyes and thoughts, this may not be the way we go at all.
NickCraver
added a commit
that referenced
this pull request
Feb 11, 2022
There seems to be just 1 case not covered by the IncludeDetailsInExceptions option on ConnectionMultiplexer - this remedies that. The option is on by default so this shouldn't break people like I thought initially. Overall, we should probably also move this option to ConfigurationOptions and defaults if we do that (#1987).
NickCraver
added a commit
that referenced
this pull request
Feb 15, 2022
There seems to be just 1 case not covered by the `IncludeDetailsInExceptions` option on `ConnectionMultiplexer` - this remedies that. The option is on by default so this shouldn't break people like I thought initially. Overall, we should probably also move this option to `ConfigurationOptions` and defaults if we do that (#1987).
I'm not 100% sure on the helper exposure here but figured it was handy - thoughts? @philon-msft here's what I had in mind for extensibility. We probably don't _need_ to override it with these defaults not, but a provider could if they wanted. IMO, no need for "lib" because _all_ connections are from a library, just felt more natural to spell our SE.Redis when doing it but we can tweak whatever - main thing is the layout here - does this make more/less sense?
Needs more, but getting initial coverage.
This makes a ServerMaintenanceEvent actually something you can implement outside the lib (TODO: assess naming), fixes Defaults cloning, and makes AfterConnectAsync more extensible (no internals anymore). Misc fixes are tidying other found things.
philon-msft
approved these changes
Feb 21, 2022
src/StackExchange.Redis/Configuration/DefaultOptionsProvider.cs
Outdated
Show resolved
Hide resolved
src/StackExchange.Redis/Configuration/DefaultOptionsProvider.cs
Outdated
Show resolved
Hide resolved
philon-msft
requested changes
Feb 22, 2022
src/StackExchange.Redis/Configuration/DefaultOptionsProvider.cs
Outdated
Show resolved
Hide resolved
philon-msft
approved these changes
Feb 22, 2022
|
@philon-msft @mgravell take note of the SSL props in here, which theoretically could be per-endpoint but are practically almost always for all (e.g. not mixing SSL and not) - should we have defaults for these? Cost is low for leaving them, but Azure for example does use SSL so seems like we should maybe change these to be private-backed full prop field combos with a nullable |
|
LGTM! |
|
@mgravell any thoughts on this one? |
mgravell
approved these changes
Mar 3, 2022
| } | ||
| } | ||
|
|
||
| return new DefaultOptionsProvider(); |
There was a problem hiding this comment.
(thought about whether to singleton this, but ... meh, it doesn't matter; leave alone, I guess - it'll be collectable that way)
There was a problem hiding this comment.
Subtle thing below in that defaultClientName does have state, that's why newing up here!
| private static readonly List<DefaultOptionsProvider> BuiltInProviders = new() | ||
| { | ||
| new AzureOptionsProvider() | ||
| }; |
There was a problem hiding this comment.
the one and only time this field is used is: in the KnownInitializers static field initializer; maybe just move this directly inline there?
There was a problem hiding this comment.
I had it this way before but it's a new nasty/confusing with the shortened new syntax it looked harder to understand, IMO - lemme add some comments!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is just an idea pitch (scrutinize heavily!) to see if the direction even works. The intent is for a consumer or wrapper library to specify a default options provider (or extend the built-in ones). For example Azure could override
IsMatchand maintain its own domain list, as could AWS, etc.Problems with this:
KnownProviderslist onDefaultOptionsProvideris awkward, butConfigurationOptionscan also get a.Defaultsset directly, or...other ideas? MaybeGetForEndpointsshould get onConfigurationOptionsas apublic staticinstead?Eyes should be given here on new public members (and naming!) on
ConnectionMultiplexer. Maintenance events are meant to be extensible as part of this change.Needs eyes and thoughts, this may not be the way we go at all but wanted to flesh it out.