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

Recognize byte slice for key argument in cluster client hash slot computation #3049

Merged
merged 4 commits into from
Nov 21, 2024

Conversation

LINKIWI
Copy link
Contributor

@LINKIWI LINKIWI commented Jul 13, 2024

This PR is mildly related to #3048.

The cluster client uses the command type to determine the expected key position in the command arguments slice to compute the hash slot. This enables the client to properly send the command to the node that owns that slot, according to the cached cluster topology state.

For generic commands assembled with the Do() API, the logic falls back to assuming the key is at position 1. Though this is a reasonable assumption, it depends on the argument to be of string type; []byte type arguments are not converted to strings, which results in an incorrect slot computation, thus forcing (in most cases) a double round trip caused by response to a MOVED redirection.

This PR adds a []byte type case for key string serialization, so that the generic commands passed as byte slices yield the correct cluster hash slot.

@LINKIWI LINKIWI changed the title Recognize byte slice for key argument in cluster client hash computation Recognize byte slice for key argument in cluster client hash slot computation Jul 13, 2024
@monkey92t
Copy link
Collaborator

It seems that you have restricted modifications to your PR code. You can either open the permissions or resolve the conflicts yourself.

@LINKIWI LINKIWI force-pushed the command-key-heuristic-bytes branch from e1f8ca8 to 7e90536 Compare July 13, 2024 11:42
@LINKIWI
Copy link
Contributor Author

LINKIWI commented Jul 13, 2024

It seems that you have restricted modifications to your PR code. You can either open the permissions or resolve the conflicts yourself.

Done! Rebased and conflicts resolved

@monkey92t
Copy link
Collaborator

@ofekshenawa merge?

@LINKIWI
Copy link
Contributor Author

LINKIWI commented Jul 22, 2024

Is it possible to merge this? This PR fixes a pretty significant performance bug for us. Thanks!

cc @vladvildanov @ofekshenawa

@LINKIWI LINKIWI force-pushed the command-key-heuristic-bytes branch from 35ae45e to 3cd7c33 Compare July 31, 2024 11:35
@LINKIWI
Copy link
Contributor Author

LINKIWI commented Jul 31, 2024

@ofekshenawa I rebased this branch and the build is now green. Are we good to merge? Thanks.

@LINKIWI
Copy link
Contributor Author

LINKIWI commented Oct 17, 2024

@vladvildanov @ofekshenawa Are you able to help with this merge? We have been running this patch in production for several months and have confirmed that it fixes the issue in the PR description.

@ofekshenawa ofekshenawa merged commit fc32d0a into redis:master Nov 21, 2024
10 checks passed
@ofekshenawa
Copy link
Collaborator

@LINKIWI Sorry for the delay, approved and merged!

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

Successfully merging this pull request may close these issues.

4 participants