Grokzen / redis-py-cluster

Python cluster client for the official redis cluster. Redis 3.0+.
https://redis-py-cluster.readthedocs.io/
MIT License
1.1k stars 316 forks source link

Make the _created_connections counter works. #464

Closed iceihehe closed 2 years ago

iceihehe commented 3 years ago

The _created_connections counter is initialized but never increased. Plus 1 when make_connection succeeds.

Grokzen commented 2 years ago

@iceihehe This change is not really needed as it is not used in this lib at all more then being reset back to 0 in one specific case/instance when calling the method reset(), but this variable is just inherited from the redis-py version of ConnectionPool but we do not use it as this ClusterConnectionPool variant use this variable instead self._created_connections_per_node where we track the same variable and value but for each individual node instead.

I will not merge this due to this reason above. If you however can show me a practical case for using this variable over the per_node one or find a bug that you can show, then we can reopen this and merge a change, but i do not see it currently.

iceihehe commented 2 years ago

It's ok. It's indeed not necessary actually. I need this variable because I just swiched from redis-py to this library and I used this variable as a metrics to reflect the saturation of redis connection pool. Of cause I can use the per node version of this variable instead.

Grokzen commented 2 years ago

After some thought, one possible improvement to this feature tho in order to keep the original redis-py code intact is to make this variable into a property on this class that would dynamically calculate this by adding up the connection count from the existing variable and just return that in the same way as redis-py does. If you make a version like that, i will consider it mergeable instead.

iceihehe commented 2 years ago

After some thought, one possible improvement to this feature tho in order to keep the original redis-py code intact is to make this variable into a property on this class that would dynamically calculate this by adding up the connection count from the existing variable and just return that in the same way as redis-py does. If you make a version like that, i will consider it mergeable instead.

I've made a change. Maybe you can have a look at it.