fix(txpipeline): keyless commands should take the slot of the keyed #3411
Conversation
ndyakov
requested review from
Copilot,
htemelski-redis,
ofekshenawa and
elena-kolevska
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@LINKIWI bringing this to your attention. |
ndyakov
force-pushed
the
ndyakov/keyless-commands-tx-pipeline
branch
from
b860a10 to
185ec01
Compare
ndyakov
force-pushed
the
ndyakov/keyless-commands-tx-pipeline
branch
from
7c977ea to
1683069
Compare
Add list of keyless Commands based on the Commands output for redis 8
ndyakov
force-pushed
the
ndyakov/keyless-commands-tx-pipeline
branch
from
1683069 to
49906ee
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the behavior in transaction pipelines so that keyless commands take the slot of keyed commands. Key changes include updating test cases to validate the new command behavior, modifying slot selection logic in the ClusterClient methods, and adding a keyless commands list in command.go to ensure proper handling of keyless commands.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| ring_test.go | Updates tests for process hook to use SET/GET instead of PING, validating proper command strings. |
| osscluster_test.go | Adds a new test to check that keyless commands execute without triggering CrossSlot errors. |
| osscluster.go | Modifies tx pipeline processing and slot selection logic by introducing a preferredRandomSlot parameter. |
| internal_test.go | Updates tests to use the new cmdSlot signature with the preferredRandomSlot parameter. |
| command.go | Introduces a keylessCommands map and updates cmdFirstKeyPos and cmdSlot to support keyless commands. |
Comments suppressed due to low confidence (2)
osscluster.go:1001
- Consider adding a comment to explain the purpose of passing -1 as the preferredRandomSlot value to improve code clarity and future maintainability.
slot := c.cmdSlot(cmd, -1)
command.go:130
- Consider updating the docstring for cmdFirstKeyPos to indicate that it returns 0 for keyless commands based on the keylessCommands table, which aids clarity for future maintainers.
func cmdFirstKeyPos(cmd Cmder) int {
| @@ -1519,8 +1526,36 @@ func (c *ClusterClient) processTxPipeline(ctx context.Context, cmds []Cmder) err | |||
| return err | |||
| } | |||
|
|
|||
| cmdsMap := c.mapCmdsBySlot(cmds) | |||
| cmdsMap := map[int][]Cmder{} | |||
There was a problem hiding this comment.
Consider refactoring the slot determination logic for keyed commands (from lines 1528-1556) into a separate helper function to enhance readability and reduce complexity.
Copilot uses AI. Check for mistakes.
Thanks, good catch. Structurally this looks good to me. I can help run this patch through our internal correctness and performance test suite early next week. |
| @@ -17,6 +17,55 @@ import ( | |||
| "github.com/redis/go-redis/v9/internal/util" | |||
| ) | |||
|
|
|||
| // keylessCommands contains Redis commands that have empty key specifications (9th slot empty) | |||
| // Only includes core Redis commands, excludes FT.*, ts.*, timeseries.*, search.* and subcommands | |||
| var keylessCommands = map[string]struct{}{ | |||
There was a problem hiding this comment.
Some other implementations ascertain this information dynamically with a call to https://redis.io/docs/latest/commands/command/ at initialization time. I don't necessarily think go-redis should do that, since it would require a command to execute successfully just for the library to initialize its internal state correctly, but it could be an interesting angle to consider, which would allow you to avoid needing to hardcode this list and maintain this source of truth manually.
There was a problem hiding this comment.
@LINKIWI I agree. We are considering this since there is other data in the Commands output that can and should be used. We will probably start with a static list and update it on initialization, but it will be part of a separate feature.
There is the possibility that there are keyless commands that can be executed on any node/slot. This should make sure that we choose the slot for the keyed commands when there is a tx pipelien request.