-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allows asyncio cluster mode connections to wait for free connection when at max. #3359
base: master
Are you sure you want to change the base?
Allows asyncio cluster mode connections to wait for free connection when at max. #3359
Conversation
@cavemanpi This functionality is already covered by https://github.com/redis/redis-py/blob/master/redis/connection.py#L1306 |
Please take a look again. What you say is valid for the synchronous client, but I don't think it's true for the asyncio client. |
@vladvildanov I updated the title to make it clearer this is in reference to the asyncio client. |
@cavemanpi Thanks for clarification! We already have a PR that aims to cover the same functionality, feel free to join as well! |
@vladvildanov, thanks for pointing me at that other PR; I appreciate it, but I find myself a little confused as I think that is trying to solve a different, but related problem. redis-py/redis/asyncio/cluster.py Line 390 in 2fb40c1
From what I can see the asyncio RedisCluster class makes use of a class called NodesManager to track cluster nodes and route commands to the correct nodes. There is not a constructor parameter to override what is used for a NodesManager class replacement. This NodesManager class has mappings to ClusterNode objects which conceptually act like both the representation of a node as well as a connection pool. There also does not look to be a public constructor parameter to override the connection pooling behavior. Is the proper fix here to actually decouple the concept of the cluster node from its connection pool and make the connection pool class configurable like most of the rest of the clients? |
@cavemanpi For alignment with sync API we have to have Line 1349 in 2fb40c1
|
Pull Request check-list
Please make sure to review and check all of these items:
NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.
Description of change
This change adds a flag to the cluster client which is propagated down to the cluster node object called 'wait_for_connections'. This helps in highly distributed high throughput environments where redis connections need to be capped to avoid running into connection limits, but also we want the request to make it through eventually in the event of some sort of stampede.