Skip to content

Conversation

@deepakverma
Copy link
Contributor

@deepakverma deepakverma commented Jun 1, 2021

A mux to a clustered cache is created by specifying one of the endpoints in the cluster. SE.Redis then runs the CLUSTER NODES command to connect to individual nodes of the cluster. The initial connection to discover cluster nodes is not used for any client application commands after the initial discovery. This PR proposes to close this connection after discovery of the nodes in the cluster, which will help to reduce the total number of connections from each of the multiplexer when running at scale.

@NickCraver
Copy link
Collaborator

Merging main in here for the connection/test fixes and a better time :)

@NickCraver
Copy link
Collaborator

Identified more testing issues (around repo permissions - new guards against cypto mining) - merging in master for a better time yet again.

…st of ips returned by cluster nodes command)
@deepakverma
Copy link
Contributor Author

Identified more testing issues (around repo permissions - new guards against cypto mining) - merging in master for a better time yet again.

Thank you @NickCraver for merging the improvements in here :)

@deepakverma
Copy link
Contributor Author

deepakverma commented Jun 22, 2021

I have fixed my code as well and now all tests are passing.
I am planning to do some more testing and add unit test coverage

@deepakverma deepakverma marked this pull request as ready for review June 22, 2021 12:25
@deepakverma
Copy link
Contributor Author

This fix will require more changes and doesn't seems to be as straight forward as I had thought where various parts of the system could depend on the discovery connection.
For example, to get cluster configuration an application could be using the discovery endpoint and this PR will be a breaking change for them.
var configString = $"{TestConfig.Current.AzureCacheServer}:6380,password={TestConfig.Current.AzureCachePassword},connectRetry=3,connectTimeout=5000,syncTimeout=5000,defaultDatabase=0,ssl=true,abortConnect=false";
var options = ConfigurationOptions.Parse(configString);
using (var conn = ConnectionMultiplexer.Connect(options))
{
conn.GetDatabase().Ping();
var nodes = conn.GetServer(options.EndPoints[0]).ClusterNodes(); //this will fail as options.EndPoints[0] is not connected anymore
Assert.Equal(nodes.Nodes.Count, conn.GetEndPoints().Length);
}

Possible fix is to introduce a configuration option that will allow client application to specify closing the discovery connection
configurationoption.CloseClusterDiscoveryConnection = true;

I am closing the PR for now and will revisit if there is a strong need for us to close this connection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants