Set cluster slot for scan commands, rather than random#2623
Merged
Conversation
36a52f2 to
f39ee1e
Compare
scan commands, rather than random
c2c460f to
3898d71
Compare
Contributor
Author
|
Hi @vmihailenco. Is there any chance you could have a look over this, and let me know if this is a change you'd be interested in? Thanks! |
3898d71 to
e0fbcea
Compare
|
Hi @vmihailenco. Just following up on this pr. Is it a change that you would consider? |
Contributor
Author
|
@chayim Hi! This is a change we've been carrying in a fork for about 6 months now. Do you have any time to have a look over and see if this is something this project would be willing to accept? The idea is basically lifted from the Redis clients for other languages. |
|
Would it be an option for scan to implicitly run over all shards if no hash-tag is included? |
Contributor
Author
|
The Java clients actually error if you don't provide a hash tag. I kinda think that behaviour is better, as it avoids the surprising result of your scan not really working at all, but it would be a breaking change for this library. |
Member
|
Hello @pete-woods , feel free to sync with master and we can check this. |
e0fbcea to
accd6ed
Compare
Contributor
Author
|
Okay - did a quick rebase |
… client - At present, the `scan` command is dispatched to a random slot. - As far as I can tell, the scanX family of commands are not cluster aware (e.g. don't redirect the client to the correct slot). - You can see [here](https://github.com/redis/jedis/blob/869dc0bb6625b85c8bf15bf1361bde485a304338/src/main/java/redis/clients/jedis/ShardedCommandObjects.java#L101), the Jedis client calling `processKey` on the match argument, and this is what this PR also does. We've had this patch running in production, and it seems to work well for us. For further thought: - Continuing looking at other Redis clients (e.g. Jedis), they outright [reject as invalid](https://github.com/redis/jedis/blob/869dc0bb6625b85c8bf15bf1361bde485a304338/src/main/java/redis/clients/jedis/ShardedCommandObjects.java#L98) any scan command that does not include a hash-tag. Presumably this has the advantage of users not being surprised when their scan produces no results when a random server is picked. - Perhaps it would be sensible for go-redis to do the same also?
accd6ed to
7b39fda
Compare
Contributor
Author
|
@ndyakov please let me know what you think, now the pr is up to date |
Member
|
@pete-woods Looks good, will double check with Jedis and see if we can include this in the next release, thank you. |
ndyakov
approved these changes
Jun 24, 2025
Member
|
After couple of discussions I don't see how this can break the current behavior, it can only improve it for when there is a hashtag in the pattern. Thank you for the contribution! |
Contributor
Author
|
Great stuff. I'd also consider erroring for cluster mode, when there's no tag set like the Java clients do, but that's a separate decision. |
ofekshenawa
pushed a commit
to ofekshenawa/go-redis
that referenced
this pull request
Jun 30, 2025
… client (redis#2623) - At present, the `scan` command is dispatched to a random slot. - As far as I can tell, the scanX family of commands are not cluster aware (e.g. don't redirect the client to the correct slot). - You can see [here](https://github.com/redis/jedis/blob/869dc0bb6625b85c8bf15bf1361bde485a304338/src/main/java/redis/clients/jedis/ShardedCommandObjects.java#L101), the Jedis client calling `processKey` on the match argument, and this is what this PR also does. We've had this patch running in production, and it seems to work well for us. For further thought: - Continuing looking at other Redis clients (e.g. Jedis), they outright [reject as invalid](https://github.com/redis/jedis/blob/869dc0bb6625b85c8bf15bf1361bde485a304338/src/main/java/redis/clients/jedis/ShardedCommandObjects.java#L98) any scan command that does not include a hash-tag. Presumably this has the advantage of users not being surprised when their scan produces no results when a random server is picked. - Perhaps it would be sensible for go-redis to do the same also? Co-authored-by: Nedyalko Dyakov <1547186+ndyakov@users.noreply.github.com>
ofekshenawa
pushed a commit
that referenced
this pull request
Aug 10, 2025
… client (#2623) - At present, the `scan` command is dispatched to a random slot. - As far as I can tell, the scanX family of commands are not cluster aware (e.g. don't redirect the client to the correct slot). - You can see [here](https://github.com/redis/jedis/blob/869dc0bb6625b85c8bf15bf1361bde485a304338/src/main/java/redis/clients/jedis/ShardedCommandObjects.java#L101), the Jedis client calling `processKey` on the match argument, and this is what this PR also does. We've had this patch running in production, and it seems to work well for us. For further thought: - Continuing looking at other Redis clients (e.g. Jedis), they outright [reject as invalid](https://github.com/redis/jedis/blob/869dc0bb6625b85c8bf15bf1361bde485a304338/src/main/java/redis/clients/jedis/ShardedCommandObjects.java#L98) any scan command that does not include a hash-tag. Presumably this has the advantage of users not being surprised when their scan produces no results when a random server is picked. - Perhaps it would be sensible for go-redis to do the same also? Co-authored-by: Nedyalko Dyakov <1547186+ndyakov@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
scancommand is dispatched to a random slot.processKeyon the match argument, and this is what this PR also does.We've had this patch running in production, and it seems to work well for us.
For further thought: