Introduce MasterReplicaClient to reuse performance-intensive resources

[grzechum]

Bug Report

Each connection created using sentinel MasterReplica.connectAsync() creates new sentinel topology provider and refresher. This heavily degrades performance when used together with connections pool.

Current Behavior

Current SentinelConnector.connectAsync() implementations create a new instance of sentinel topology provider and refresher for every connection.

    @Override
    public CompletableFuture<StatefulRedisMasterSlaveConnection<K, V>> connectAsync() {

        TopologyProvider topologyProvider = new SentinelTopologyProvider(redisURI.getSentinelMasterId(), redisClient, redisURI);
        SentinelTopologyRefresh sentinelTopologyRefresh = new SentinelTopologyRefresh(redisClient,
                redisURI.getSentinelMasterId(), redisURI.getSentinels());

        MasterSlaveTopologyRefresh refresh = new MasterSlaveTopologyRefresh(redisClient, topologyProvider);
        MasterSlaveConnectionProvider<K, V> connectionProvider = new MasterSlaveConnectionProvider<>(redisClient, codec,
                redisURI, Collections.emptyMap());

        Runnable runnable = getTopologyRefreshRunnable(refresh, connectionProvider);

        return refresh.getNodes(redisURI).flatMap(nodes -> {

            if (nodes.isEmpty()) {
                return Mono.error(new RedisException(String.format("Cannot determine topology from %s", redisURI)));
            }

            return initializeConnection(codec, sentinelTopologyRefresh, connectionProvider, runnable, nodes);
        }).onErrorMap(ExecutionException.class, Throwable::getCause).toFuture();
    }

Expected behavior/code

Create single sentinel topology provider and refresher for all connections created from same client and URI.

Environment

• Lettuce version(s): 5.2.2.RELEASE
• Redis version: 5.0.x

[mp911de]

We currently have no way to reuse these facilities as the factory exposes static methods only. We didn’t add a stateful object such as RedisClusterClient to avoid yet another client object.

[grzechum]

Would it be possible to create external SentinelConnector instance (that will encapsulate single sentinel topology and refresher) and pass it to MasterReplica.connectAsync() as optional parameter?
This way backward compatibility may be preserved.

SentinelConnector requires same arguments as MasterReplica.connectAsync() so it can be created upfront and used to create connections in connection pool.

[mp911de]

These resources may be associated with state and disposable resources (e.g. the sentinel variant maintains connections to Redis sentinels to capture topology changes via pub/sub). We’ve a few tickets that as for master replica extensions that do not play well with the static factory methods.

We will eventually have to add a something like a MasterReplica client and deprecate the MasterReplica factory. One of the items is the ability to use topology refresh on demand and a single connection isn’t the right place to do so.

[grzechum]

Do you see any workaround which we may apply, that will work with 5.2.x ?

[mp911de]

No, we cannot do anything for 5.2.x and 5.3. The earliest version with such support could be Lettuce 6 and even for Lettuce 6 it's unclear whether it's going to be 6.0 or 6.1. Given the current Corona impact I have even less time to build features myself.