only send readonly to replicas once on new connections

[duke-cliff]

replicas have more CPU load because of extra commands(2x) than masters.

[Grokzen]

@duke-cliff Most tests fail with this current solution.Seems like the core solution/idea do not work as you expect in your commit there.

[Grokzen]

@duke-cliff Also how is this implementation different from the following code lines that exists in the on_connect method? https://github.com/Grokzen/redis-py-cluster/blob/6f1fb57c9522ce65ccea7d1a75b1118a32634d02/rediscluster/connection.py#L62

[duke-cliff]

@Grokzen I don't think that code works.

1. It's only working for readonly=True. However, we need to check read_from_replicas=True
2. It's getting the value from kwargs: https://github.com/Grokzen/redis-py-cluster/blob/6f1fb57c9522ce65ccea7d1a75b1118a32634d02/rediscluster/connection.py#L51 But it won't even pass the named params(including read_only/read_from_replcias) from here: https://github.com/Grokzen/redis-py-cluster/blob/2.1.0/rediscluster/client.py#L375
3. In the ClusterConnection class, I think it might be too late because the connection object itself does not know the node information which contains whether the spawned connection is from a master node or a slave node. Unless you have some different thoughts, e.g to pass the node info to every connection object.

[Grokzen]

@duke-cliff Actually it will be passed down if i traced the code correct but the path is a bit strange :)

Setting readonly_mode=True into the RedisCluster class will change the connectionpool class to ClusterReadOnlyConnectionPool and it hardcodes readonly=True when it calls the super() function to ClusterConnectionPool which in turn is bound into connection_kwargs in the ClusterConnectionPool and those kwargs should be set/sent into each ClusterConnection class and it should be tracked for each connection created.

But... the downside with this is that if you set readonly mode, then you will send readonly to all nodes in the cluster when ever any connection is created. Even master nodes which technically is redundant but should not hurt in reality.

I do like this implementation as well, as it properly only sends it to slave nodes and it might work with all subclassed cluster connection pools as well as it moves the logic away from the connection side of things into the generic connection pool which is good, and the hack/workaround in the main client execute logic is removed which i like as well :)

[dustMason]

Hi, we are experiencing this issue as well! Let me know if there's anything I can do to help get this merged.

[dustMason]

I'm reviewing this thread and the associated branch now – based on what you said above @Grokzen, does this mean we can simply remove these lines and instead rely on the READONLY command that gets sent during connection init here, as you describe?