Using Redis with sentinels #11617

[mazuryv]

PR is created.

Fixes #11617

[dnfadmin]

All CLA requirements met.

[jtkech]

Maybe we could keep the same pattern in the startup, meaning still doing a parse in a try/catch but just to validate the config string once and before registering the services, so here the only update would be to set options.Configuration to originalConfigurationString that you can rename to configurationString.

Then, looks good to parse it in the ConfigurationOptions getter.

What you think about this comment dotnet/aspnetcore#28367 (comment) saying the following

However, once the master fails and another master is elected by the sentinels, this connection object is no longer valid and the caching breaks. There is currently no way to re-instantiate the connection easily.

[mazuryv]

I check it:

However, once the master fails and another master is elected by the sentinels, this connection object is no longer valid and the caching breaks. There is currently no way to re-instantiate the connection easily.
When sentinels elect new master from slave - looks like connection object is still valid and connect to another server without reconection.

[mazuryv]

I use options.Configuration because same value is used in RedisCacheOptions

namespace Microsoft.Extensions.Caching.StackExchangeRedis
{
    public class RedisCacheOptions : IOptions<RedisCacheOptions>
    {
        /// <summary>
        /// The configuration used to connect to Redis.
        /// </summary>
        public string Configuration { get; set; }
        ...
    }
} 

I think try/catch protected only against parse exception, not against bad configuration.
When public ConfigurationOptions ConfigurationOptions => ConfigurationOptions.Parse(Configuration); is used, we always get correct configuration.

[jtkech]

Okay cool, thanks for trying it.

I think try/catch protected only against parse exception, not against bad configuration.

Yes but still useful to do as a first level check before registering the related services. But yes keep the change to set the Configuration string options.Configuration = configurationString.

When public ConfigurationOptions ConfigurationOptions => ConfigurationOptions.Parse(Configuration); is used, we always get correct configuration.

Then yes, also keep this change.

Need to leave, will look at it again this night.

[sebastienros]

Can you keep the error message in a different param? That might help investigate failures

[mazuryv]

Ok. I kept it.

[jtkech]

@mazuryv I approved

Just 2 comment suggestions to be applied.

[jtkech]

@mazuryv I approved

Just 2 comment suggestions to be applied before merging

Edited: Just committed the 2 little suggestions ;)

[jtkech]

Last thing, please add a new line between each property, I can't edit your file.

Okay done, I could do it by making a suggested code and then committing it.

[jtkech]

@mazuryv Last thing, please add a new line between each RedisOptions property, I can't edit your file.

Okay done, I could do it by making a suggested code and then committing it.

[mazuryv]

Sorry, I was offline.
I prepare little cosmetic change if you want to approve it.

[jtkech]

No problem

I prepare little cosmetic change if you want to approve it.

I already approved it.

Only need to wait for the next triage ;)

[sebastienros]

What are the breaking changes to document with this PR?
Should users change some settings name after that PR? And would it just build without any change on source code?

[jtkech]

There is no breaking change, we still generate ConfigurationOptions objects by parsing the Configuration string but lazily. The problem was that when connecting with sentinels the ConfigurationOptions is mutated, so e.g. after our service was connected, redis cache was trying to connect with a bad ConfigurationOptions, that we now generate lazily from the Configuration string.