-
Notifications
You must be signed in to change notification settings - Fork 6
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
Thread-safe implementation of the sentinel connection pool #4
base: master
Are you sure you want to change the base?
Conversation
flask_redis_sentinel.py
Outdated
yield self.get_master_address() | ||
except MasterNotFoundError: | ||
pass | ||
raise SlaveNotFoundError('No slave found for %r' % (self.service_name)) |
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 cant find MasterNotFoundError and SlaveNotFoundError definied anywhere
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.
Also redundant parentheses
flask_redis_sentinel.py
Outdated
return master_address | ||
|
||
def rotate_slaves(self): | ||
"Round-robin slave balancer" |
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.
triple quotes should be used for doc string (on multiple methods)
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 was copying the surrounding and didn't notice this. 🤦♂️
flask_redis_sentinel.py
Outdated
return super(SentinelManagedConnection, self).read_response() | ||
except ReadOnlyError: | ||
if self.connection_pool.is_master: | ||
# When talking to a master, a ReadOnlyError when likely |
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 think 'when' after ReadOnlyError should be removed
flask_redis_sentinel.py
Outdated
self.get_master_address() | ||
return False | ||
if self.is_master: | ||
if self.master_address != (connection.host, connection.port): |
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 am not sure here but, is it intentional to use self.master_address
instead of self.get_master_address()
? Maybe self.master_address
should also be a private?
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.
Yes, that's intentional. self.get_master_address()
connects to redis sentinel and resolves the current master, self.master_address
is a cache from the last call. The resolution is not something that should happen every single time you take/return a connection from the pool.
This now depends on https://github.com/exponea/redis-py/releases/tag/2.10.6-exponea-1 |
Thread-safe implementation of the sentinel connection pool, so only one pool per process is now used.
Upstream pull request:
redis/redis-py#909