-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix(txpipeline): keyless commands should take the slot of the keyed #3411
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
Conversation
@LINKIWI bringing this to your attention. |
b860a10
to
185ec01
Compare
7c977ea
to
1683069
Compare
Add list of keyless Commands based on the Commands output for redis 8
1683069
to
49906ee
Compare
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. |
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.
Overall looks good. Maybe we can improve the performance and the readability a bit
072d8d8
to
57a57be
Compare
57a57be
to
2bbcdaa
Compare
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.
Pull Request Overview
This PR ensures that keyless commands in a transaction pipeline use the same slot as keyed commands by introducing a preferred random slot concept and filtering keyed commands for slot determination.
- Extend
cmdSlot
to accept apreferredRandomSlot
and propagate it through the pipeline mapping logic - Add
slottedKeyedCommands
helper to group keyed commands by slot - Update tests to cover the preferred slot behavior and pipeline execution of keyless commands
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
ring_test.go | Adjusted Process-hook tests to invoke Get instead of Ping and updated expectations |
osscluster_test.go | Added a test to verify keyless commands in a pipeline don’t trigger ErrCrossSlot |
osscluster.go | Refactored slot resolution: added preferredRandomSlot parameter and slottedKeyedCommands |
internal_test.go | Added tests for new preferredRandomSlot behavior in cmdSlot |
command.go | Introduced keylessCommands map and updated cmdFirstKeyPos to recognize keyless commands |
Comments suppressed due to low confidence (1)
osscluster.go:1583
- [nitpick] The function name
slottedKeyedCommands
is a bit unclear. Consider renaming it tokeyedCommandsBySlot
to more directly convey that it returns a map of slot → keyed commands.
func (c *ClusterClient) slottedKeyedCommands(cmds []Cmder) map[int][]Cmder {
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.