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

Implement locks for RedisCluster #2013

Merged
merged 9 commits into from
Mar 1, 2022

Conversation

jakebarnwell
Copy link
Contributor

@jakebarnwell jakebarnwell commented Feb 22, 2022

Pull Request check-list

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

  • Does $ tox pass with this change (including linting)?
  • 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)?
  • Is the changes 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

Adds support for the lock() method on RedisCluster. Currently, only Redis class supports lock(). #1937 added lua scripting support for RedisCluster which is a prerequisite for locking support, so with that out of the way, this PR finishes adding support for the .lock() method.

The change is relatively simple: we just copy and paste the Redis.lock() implementation onto RedisCluster.lock(). However, we do change the Lock implementation for acquiring the encoder if necessary. Instead of calling self.redis.connection_pool.get_encoder(), we now call self.redis.get_encoder(). Both of these methods already exist so it shouldn't be breaking (?).

@codecov-commenter
Copy link

codecov-commenter commented Feb 22, 2022

Codecov Report

Merging #2013 (50d1b9a) into master (afc83e1) will decrease coverage by 0.00%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2013      +/-   ##
==========================================
- Coverage   92.62%   92.62%   -0.01%     
==========================================
  Files         100      100              
  Lines       20910    20913       +3     
==========================================
+ Hits        19368    19370       +2     
- Misses       1542     1543       +1     
Impacted Files Coverage Δ
redis/asyncio/client.py 90.45% <ø> (ø)
redis/client.py 89.39% <ø> (ø)
tests/test_lock.py 100.00% <ø> (ø)
redis/cluster.py 92.43% <80.00%> (-0.08%) ⬇️
redis/lock.py 100.00% <100.00%> (ø)
tests/test_cluster.py 98.29% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update afc83e1...50d1b9a. Read the comment docs.

@@ -224,7 +224,7 @@ def owned(self):
# need to always compare bytes to bytes
# TODO: this can be simplified when the context manager is finished
if stored_token and not isinstance(stored_token, bytes):
encoder = self.redis.connection_pool.get_encoder()
encoder = self.redis.get_encoder()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm beginning to confuse myself a bit -- I think this is a safe change since both redis classes have the .get_encoder() method defined on them already? But I'm not positive

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're right. Since merging in cluster and asyncio, we actually need to start cleaning and unifying these interfaces. There's a lot of duplication - but the philosophy was first get it in, then clean it up.
@dvora-h concur?

@jakebarnwell jakebarnwell changed the title Add support for .lock() for RedisCluster Implement .lock() method on RedisCluster Feb 22, 2022
@@ -94,10 +96,10 @@
* Removing command on initial connections (#1722)
* Removing hiredis warning when not installed (#1721)
* 4.0.0 (Nov 15, 2021)
* FT.EXPLAINCLI intentionally raising NotImplementedError
* FT.EXPLAINCLI intentionally raising NotImplementedError
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for these whitespace trims -- my IDE does it 😅

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flake8 actually enforces it, but only on python files ;)

@jakebarnwell
Copy link
Contributor Author

@dvora-h see my comment on #1898 -- my original PR #1937 technically didn't solve the issue since the .lock() method wasn't implemented in that PR, but this PR does implement the .lock() method 😃

@chayim chayim self-requested a review February 24, 2022 14:16
@chayim chayim added the feature New feature label Feb 24, 2022
@chayim chayim changed the title Implement .lock() method on RedisCluster Implement locks for RedisCluster Feb 24, 2022
@jakebarnwell
Copy link
Contributor Author

I think the tests are flaky 🤔 -- they pass fine on my machine, at least

@dvora-h
Copy link
Collaborator

dvora-h commented Mar 1, 2022

Awesome @jakebarnwell, thank you for the PR! I merge it into master.

@dvora-h dvora-h merged commit 6149004 into redis:master Mar 1, 2022
@long2ice
Copy link

long2ice commented Oct 2, 2022

What about asyncio cluster client?

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

Successfully merging this pull request may close these issues.

6 participants