Add cluster support for scripting #1937
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1937 +/- ##
==========================================
- Coverage 92.69% 92.64% -0.05%
==========================================
Files 100 100
Lines 20825 20909 +84
==========================================
+ Hits 19303 19371 +68
- Misses 1522 1538 +16
Continue to review full report at Codecov.
|
redis/commands/core.py
Outdated
| @@ -4663,7 +4663,7 @@ def __init__(self, registered_client, script): | |||
| if isinstance(script, str): | |||
| # We need the encoding from the client in order to generate an | |||
| # accurate byte representation of the script | |||
| encoder = registered_client.connection_pool.get_encoder() | |||
| encoder = registered_client.get_encoder() | |||
There was a problem hiding this comment.
This is technically a breaking change. If any consumers of redis-py use Scripts directly, injecting their own registered_client that isn't a subclass of Redis or RedisCluster, this could break for them if they haven't got client.get_encoder() defined.
There was a problem hiding this comment.
While I believe it's very unlikely - I'd rather not break the universe. Perhaps we could wrap this with an if getattr, check. What do you think @jakebarnwell
There was a problem hiding this comment.
That's a good idea! I will try something like that
| r.set("a", 2) | ||
| # 2 * 3 == 6 | ||
| assert r.eval(multiply_script, 1, "a", 3) == 6 | ||
|
|
||
| # @skip_if_server_version_lt("7.0.0") turn on after redis 7 release | ||
| @pytest.mark.onlynoncluster | ||
| def test_eval_ro(self, unstable_r): |
There was a problem hiding this comment.
I haven't implemented eval_ro -- I'm not sure if this PR desires it?
There was a problem hiding this comment.
I don't think you should have to deal with this.
|
@chayim can you do another review? :) |
|
@jakebarnwell This is a beautiful thing. Thank you for the approach, and the changes. I've approved, and we'll merge this into master shortly. It'll be part of the 4.2.0rc1 release. |
|
@dvora-h Can you have a look at the conflict here and resolve it for our friend @jakebarnwell prior to the 4.2.0rc1 release? |
Pull Request check-list
Please make sure to review and check all of these items:
$ toxpass with this change (including linting)?NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.
Description of change
Partly fixes #1898:
Adds scripting support, but does not yet add
.lock()toRedisCluster. That will come in a following PR.Much of this change was
stolenadapted fromredis-py-cluster.Implements the following commands:
EVAL <SCRIPT> num_keys KEY1 ... KEYN ...); the keys must all be on the same node. If the script requires 0 keys, the command is sent to a random node.I have not (yet?) implemented the
EVAL_ROcommands -- do those need to be implemented? Maybe.This PR does not implement, nor do I intend to implement:
I have added a couple unit tests for the newly-implemented things.
I have not yet added documentation to the PR, but I will.