Skip to content
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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

cavemanpi
Copy link

@cavemanpi cavemanpi commented Aug 21, 2024

Pull Request check-list

Please make sure to review and check all of these items:

  • Do tests and lints pass with this change?
  • Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Is there an example added to the examples folder (if applicable)?
  • Was the change added to CHANGES file?

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.

@vladvildanov
Copy link
Collaborator

@cavemanpi This functionality is already covered by BlockingConnectionPool, you should use RedisCluster with provided configuration connection_pool_class=BlockingConnectionPool

https://github.com/redis/redis-py/blob/master/redis/connection.py#L1306

@cavemanpi
Copy link
Author

@cavemanpi This functionality is already covered by BlockingConnectionPool, you should use RedisCluster with provided configuration connection_pool_class=BlockingConnectionPool

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.

@cavemanpi cavemanpi changed the title Allows cluster mode connections to wait for free connection when at max. Allows asyncio cluster mode connections to wait for free connection when at max. Sep 5, 2024
@cavemanpi
Copy link
Author

@vladvildanov I updated the title to make it clearer this is in reference to the asyncio client.

@vladvildanov
Copy link
Collaborator

@cavemanpi Thanks for clarification! We already have a PR that aims to cover the same functionality, feel free to join as well!

#3321

@cavemanpi
Copy link
Author

cavemanpi commented Sep 6, 2024

@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.

self.nodes_manager = NodesManager(

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?

@vladvildanov
Copy link
Collaborator

@cavemanpi For alignment with sync API we have to have connection_pool_class argument in constructor, so this way it would be possible to set a type for connection pool. Use sync API as an example

connection_pool_class=ConnectionPool,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants