ConfigurationOptions: observe modifications after ConnectionMultiplexer start #2050
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
- Moves IncludeDetailInExceptions and IncludePerformanceCountersInExceptions from ConnectionMultiplexer to ConfigurationOptions (with backwards compatible APIs). - Move to a cleaner TryGetTieBreaker approach instead of Sentinel defaults needing to set it.
This needs a suite of tests but basically we're cloning the bits we don't want to fork from RawConfig (ConfigurationOptions ref on ConnectionMultiplexer). This may seem minor, but the net impact is a user can hold on to a ConfigurationOptions passed in and further modify it, e.g. to rotate passwords, etc. upstream. A few things we still clone are called out explicitly in the ConfigurationOptions docs.
This hasn't worked in some time - properly [Obsolete] it and point to the effective option on SocketManagerOptions.
NickCraver
changed the title
Cleaning these up for a 3.x move down the road.
philon-msft
approved these changes
Mar 20, 2022
NickCraver
changed the title
6 tasks
mgravell
approved these changes
Mar 25, 2022
NickCraver
added a commit
that referenced
this pull request
Aug 19, 2023
ILogger support - since we support `Action<ConfigurationOptions>` this can be added today e.g. with `o => o.LoggerFactory = myLoggerFactory`:
```cs
var muxer = ConnectionMultiplexer.Connect("localhost", o => o.LoggerFactory = myLoggerFactory);
```
...or we could add sibling overloads to the existing `TextWriter`. I'd love to not create a ^2 matrix every time we do this, though. Starting simpler for that reason. Note: this is on top of #2050 for simplicity, will roll against `main` when merged.
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.
Overall changes:
ConfgurationOptionspassed into aConnectionMultiplexer.Connect*()method is no longer cloned. This means it can be modified after connecting and changes will take effect on the next access. For example a user can rotate passwords or change timeouts on the fly.EndPointsare cloned and memoized toConnectionMultiplexerand changing them is not respected because this creates paradoxes and races in the multiplexer. These are explicitly called out inConfigurationOptionsdocs.IncludeDetailInExceptionsandIncludePerformanceCountersInExceptionsfromConnectionMultiplexertoConfigurationOptions(with backwards compatible APIs).[Obsolete]now?TryGetTieBreakerapproach instead of Sentinel defaults needing to set it.SetDefaultPortsis now more cleanly based onServerType(rather than asentinelbool everywhere...that felt icky.TODO:
ConfigurationOptionsmemoized property to make clear it was copied and has no effect after connection.ConnectionMultiplexer.IncludeDetailInExceptionsandConnectionMultiplexer.IncludePerformanceCountersInExceptions?