Repository navigation
Skip COMMAND GETKEYS for movablekeys commands with complete key specs - #4605
petyaslavova merged 13 commits into
Conversation
The cluster client sent COMMAND GETKEYS before every movablekeys command (ZUNIONSTORE, LMPOP, SINTERCARD, ...) just to find the slot. The key specs from COMMAND already describe where the keys are; read them locally and only ask the server when a spec is incomplete or uses a keyword search (XREAD, SORT, MIGRATE, GEORADIUS).
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4813005920
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 4813005. Configure here.
MSETEX counts keys, not arguments, so only its first key was returned.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 262c4576fb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…to perf-cluster-movablekeys-specs
petyaslavova
left a comment
petyaslavova
left a comment
There was a problem hiding this comment.
Hey @patrick-immometrica, thank you for your contribution!
Skipping the extra round trip for movablekeys commands is a good improvement, and I would like to merge it. A few changes are needed first:
- Match the server for the two spec flags it treats specially.
COMMAND GETKEYSskips specs flaggednot_key, and it does not use the specs when one is flaggedvariable_flags. Please skipnot_keyspecs and returnNone(ask the server) onvariable_flags. The existing_key_spec_flagshelper in the same file already reads the flags in both RESP2 and RESP3 shapes, so please reuse it. - This runs for every cluster command with movable keys, so please parse the specs once in
initialize()(both parsers) instead of on every call. A per-command tuple, or a marker meaning "ask the server", is enough. Theargs[0].split()copy can go too, becauseget_keysalready splits multi-word command names. - Please add the async counterparts of the two new tests in
tests/test_asyncio/test_command_parser.py. - Please update the text that still says these keys come from
COMMAND GETKEYS: theCommandsParserandAsyncCommandsParserdocstrings,docs/resp3_features.rst, andAGENTS.md.
petyaslavova
added
maintenance
Parse the key specs of every movablekeys command when the parser initializes, for both the sync and the async parser, instead of on every get_keys() call. Match COMMAND GETKEYS for the two flags it treats specially: specs flagged not_key are skipped, and a command with a spec flagged variable_flags is left to the server. The args[0].split() copy is gone, since get_keys() already splits multi-word command names. Add the async counterparts of the key-spec tests, and update the parser docstrings, docs/resp3_features.rst and AGENTS.md.
petyaslavova
left a comment
petyaslavova
left a comment
There was a problem hiding this comment.
Thanks for the quick update. All four points are addressed, and the compiled specs match the server's key-spec handling.
One small thing remains: a few comments still say the cluster client finds movablekeys keys through COMMAND GETKEYS. Please update the comment above _CACHEABLE_KEYED in redis/commands/metadata.py, the slot_routed_args and TestWithheldRoutingPolicies docstrings in tests/test_command_metadata.py, and the TestStaticMetadataRouting docstrings in tests/test_cluster.py and tests/test_asyncio/test_cluster.py. After that, this is good to go once CI passes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76666b6e0b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
int() accepts "+2", " 2", "02", "2_0" and the float 2.5, which the server rejects, so the spec path routed commands the server would refuse as malformed. Only a count in its canonical form is read locally now.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5bdcf53569
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
A str key count is checked after encoding, since a non-ASCII client encoding changes the bytes the server parses. A keynum or range spec whose last key falls before the first now yields no keys, as on the server, instead of a slice that wraps around.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95bb27a2e1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The server checks the argument count before it reads key specs, so a call below or off the command's arity no longer gets keys from the specs. A key count with more than 19 digits is out of range for the server and made int() raise past 4300 digits; it now falls back too.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57350b6857
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
petyaslavova
left a comment
petyaslavova
left a comment
There was a problem hiding this comment.
Thanks for the follow-up commits. The comment updates are in, and the new count, arity and range checks match how the server reads key specs.
One small change before I merge: _get_keys_from_specs reads and casts self.commands[cmd_name]["arity"] on every call, but the arity is already an int and never changes per command. Please store it with the compiled specs in _compile_movablekeys_specs (for example as (arity, specs)), so the hot path skips the extra lookup.
|
Done, |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1971410eaa
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
petyaslavova
left a comment
petyaslavova
left a comment
There was a problem hiding this comment.
Thanks, the arity is now compiled with the specs and the hot path only unpacks it. CI is green. Approving.
Description of change
For a
movablekeyscommand such as ZUNIONSTORE, ZINTER, LMPOP, BZMPOP or SINTERCARD,CommandsParser.get_keys()sendsCOMMAND GETKEYSto the default node. The cluster client only needs the keys to pick a slot, so every one of these commands costs an extra round trip first. AClusterPipelinepays that round trip once per queued command.The key specs we already cache from
COMMANDsay where the keys are.get_keys()now reads them the same way the server does, handlingindexbegin_search withrangeorkeynumfind_keys. It still asks the server when a spec isincompleteor useskeyword/unknown, which covers XREAD, SORT, MIGRATE and GEORADIUS. The helper sits in the shared base class, so the async parser uses it too.get_keys()per call against a local server, so this is just the loopback round trip. Over a real network the gap is larger:I checked the result against
COMMAND GETKEYSfor about 25 commands on RESP2 and RESP3, with and withoutdecode_responses, and got no mismatches. One behaviour change: keys found this way are returned as the caller passed them, the same as the fixed-position path already does, rather than in the server's encoding. I adjusted two assertions intest_get_moveable_keysfor that.Pull Request check-list
Note
Medium Risk
Changes cluster slot routing for movablekeys commands; incorrect spec compilation could mis-route before MOVED redirects, though tests and GETKEYS fallback mitigate that.
Overview
Cluster routing for
movablekeyscommands (ZUNIONSTORE,LMPOP,SINTERCARD, etc.) no longer always pays aCOMMAND GETKEYSround trip:CommandsParser/AsyncCommandsParsercompile key specs fromCOMMANDoninitialize(), andget_keys()resolves keys locally via the new_get_keys_from_specspath (range/keynum, server-style arity andnumkeysvalidation).COMMAND GETKEYSremains the fallback for incomplete specs, unsupported searches, or malformed argument counts.Keys returned from the spec path keep the caller’s argument types (strings/bytes), matching fixed-position extraction rather than the server’s encoding.
key_slot()now acceptsmemoryviewkeys. Docs and tests were extended for sync/async parsers and cluster commentary.Reviewed by Cursor Bugbot for commit 3ce6eb6. Bugbot is set up for automated code reviews on this repo. Configure here.