-
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
Enable AsyncIO cluster mode lock #2446
Conversation
7bd5daf
to
7c6b004
Compare
Codecov ReportBase: 92.05% // Head: 92.03% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #2446 +/- ##
==========================================
- Coverage 92.05% 92.03% -0.03%
==========================================
Files 110 110
Lines 28705 28721 +16
==========================================
+ Hits 26425 26433 +8
- Misses 2280 2288 +8
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
7c6b004
to
4dafb58
Compare
Maybe we can simplify my change by using def get_encoder(self):
"""Get the connection pool's encoder"""
return self.connection_pool.get_encoder() and redis/commands/core.py def get_encoder(self):
"""Get the encoder to encode string scripts into bytes."""
try:
return self.registered_client.get_encoder()
except AttributeError:
# DEPRECATED
# In version <=4.1.2, this was the code we used to get the encoder.
# However, after 4.1.2 we added support for scripting in clustered
# redis. ClusteredRedis doesn't have a `.connection_pool` attribute
# so we changed the Script class to use
# `self.registered_client.get_encoder` (see above).
# However, that is technically a breaking change, as consumers who
# use Scripts directly might inject a `registered_client` that
# doesn't have a `.get_encoder` field. This try/except prevents us
# from breaking backward-compatibility. Ideally, it would be
# removed in the next major release.
return self.registered_client.connection_pool.get_encoder() |
4dafb58
to
81a8d41
Compare
This comment was marked as outdated.
This comment was marked as outdated.
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.
LGTM!
Pull Request check-list
Please make sure to review and check all of these items:
$ tox
pass with this change (including linting)?Description of change
Enable AsyncIO cluster mode lock. AsyncIO Cluster mode has not been providing
Lock
, due to the fact that thecore.py
trying to find theconnection_pool
attribute where it's not provided in the AsyncIO cluster client. This change will try to findget_encoder
in the client ifconnection_pool
is unavailable.Also, this change copies the
lock
method of AsyncIO client (please let me know if this is not appropriate), providing the same API to the users whether they are using non-cluster or cluster.An example is not added as there is no currently available cluster-mode-only example for asyncio Redis.
Closes: #2420