Allow providing custom ClusterTopologyRefresh implementation.

[alessandrosimi-sa]

Feature Request

Is your feature request related to a problem? Please describe

During our load tests we have noticed an increasing level of CPU in each node of our Redis cluster that was proportional to the number of server connected.

Each server was on idle (no requests) and had a single Redis lettuce client instance running.

By looking at the Redis metrics we noticed that each node was constantly accepting new connections and reading the command logs we narrowed down the cause to the cluster topology refresh requests.

The DefaultClusterTopologyRefresh class creates new connection every time is called.

We have already increased the cluster topology refresh and we have switched the dynamicRefreshSources off, so the client refresh the topology is done against the seeds, in our case a single node.

With this adjustments we saw improvements but the CPU problem is still there.

This limits our ability to scale the number of server instances.

Describe the solution you'd like

Modify the DefaultClusterTopologyRefresh class to keep the connection alive so they can be re-used between topology refresh calls.

Maybe the ConnectionTracker class can be leveraged to store the connections between the calls.

Teachability, Documentation, Adoption, Migration Strategy

I will be happy to implement this feature, but I would like to know your opinion and maybe suggestions around the implementation.

Thanks

[mp911de]

The design of the topology refresh follows the idea that a connection is expensive as it allocates memory and CPU on the server-side. These resources should be rather left for actual clients that want to do work.

The built-in topology refresh is a default implementation that works for a majority of users. You can update and track the topology externally if the built-in mechanism isn't meeting your requirements.

[alessandrosimi-sa]

Thanks Mark for the quick response. You have a good point around the default implementation.

Would it possible to provide a mechanism/way to replace the ClusterTopologyRefresh interface implementation?

By looking at the RedisClusterClient class the refresh property is final.

public class RedisClusterClient extends AbstractRedisClient {
  // ...
  private final ClusterTopologyRefresh refresh = ClusterTopologyRefresh.create(new NodeConnectionFactoryImpl(),
            getResources());
  // ...
}

What do you think to remove final and have a method that sets the property?

public void SetClusterTopologyRefresh(ClusterTopologyRefresh refresh) {
  this.refresh = refresh;
}

[mp911de]

I think we can provide a delayed initialization along with a template method that can be overridden by subclasses. The refresh is invoked from different threads so making it mutable is prone to introducing visibility issues.

[mp911de]

That's in place now.

[alessandrosimi-sa]

Nice, thanks a lot!