-
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
feat: Implemented redis.asyncio.SentinelBlockingConnectionPool. #3321
base: master
Are you sure you want to change the base?
feat: Implemented redis.asyncio.SentinelBlockingConnectionPool. #3321
Conversation
c8eecd5
to
713fa03
Compare
713fa03
to
baf2af2
Compare
baf2af2
to
9701492
Compare
ed788bd
to
6539da1
Compare
aca1d65
to
f6168c3
Compare
f6168c3
to
9b1fbd6
Compare
@@ -97,6 +107,55 @@ class SentinelManagedSSLConnection(SentinelManagedConnection, SSLConnection): | |||
pass | |||
|
|||
|
|||
class SentinelConnectionPoolProxy: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of this proxy class? get_master_address
and rotate_slaves
methods implementations are exactly the same as in SentinelConnectionPool
class. The only difference that I see is the missing calls to parent object in constructor and reset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it this way because it is already done in the synchronous version: https://github.com/redis/redis-py/blob/master/redis/sentinel.py#L89
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I get it. It's a bad design approach and I don't want to spread it even more. It should be implemented correctly and I will create an issue for the future to do the same with sync version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. I'll think about a better solution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The best solution will be refactor async ConnectionPool the way we don't need to use workarounds to break inheritance limitations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello. What if we move BlockingConnectionPool logic to ConnectionPool?
We can do something like this:
from typing import Optional
# If the timeout parameter is zero, then we have the old behavior,
# else we have a blocking connection pool
class ConnectionPool:
def __init__(
self,
timeout: Optional[int] = 0,
**kwargs,
) -> None:
...
# For backward compatibility
class BlockingConnectionPool(ConnectionPool):
def __init__(self, timeout: Optional[int] = 20, **kwargs) -> None:
super().__init__(timeout=timeout, **kwargs)
@@ -116,12 +175,17 @@ def __init__(self, service_name, sentinel_manager, **kwargs): | |||
) | |||
self.is_master = kwargs.pop("is_master", True) | |||
self.check_connection = kwargs.pop("check_connection", False) | |||
self.proxy = SentinelConnectionPoolProxy( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, conceptually proxies are used to add an additional functionality before or after we want to access the original object. But here proxy are used just to override super class methods that breaks the functionality of child.
Proxy here is the workaround, to hide a problem with inheritance that we have. We have to either fix it in ConnectionPool so it can be inheritable, or doesn't inherit this at all
@vladvildanov suggested I come over here as well. I have a PR which seems tangentially related. Any others care to give feedback on the idea expressed in this comment? #3359 (comment) |
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
Added redis.asyncio.SentinelBlockingConnectionPool class