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.

Sign up for GitHub

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

Jump to bottom

GetHashSlot for XREADGROUP, XREAD, XGROUP and XINFO is incorrect #2086

Closed
nielsderdaele opened this issue Apr 13, 2022 · 1 comment
Closed

GetHashSlot for XREADGROUP, XREAD, XGROUP and XINFO is incorrect #2086

nielsderdaele opened this issue Apr 13, 2022 · 1 comment

Comments

@nielsderdaele
Copy link
Contributor

The current implementation of XREADGROUP, XREAD, XGROUP and XINFO uses the ComandValuesMessage this results in an incorrect HashSlot of -1. Using these commands in a cluster environment will result in unnecessary MOVED responses (and round trips).

To fix these command some additional Message types are required. XGROUP and XINFO could be implemented using a CommandValueKeyValuesMessage (first argument is a value, second argument the key and followed by one or more values).

The XREADGROUP and XREAD commands will require a custom Message as they have multiple keys near the end of the command after the STREAMS argument. In redis-server a custom getKeys function has been implemented as well for these commands (see xreadGetKeys in db.c)
This was referenced Apr 13, 2022
Merged
Merged
NickCraver added a commit that referenced this issue Jun 14, 2022
@nielsderdaele @NickCraver
Fix incorrect HashSlot calculation for XREAD and XREADGROUP commands (#…
8c9cb1b 
…2093)

I have created a fix for the incorrect HashSlot calculation for the XREAD and XREADGROUP commands (#2086) Since these differ a lot from other messages I have created some new CommandMessage classes:
- SingleStreamReadGroupCommandMessage 
- MultiStreamReadGroupCommandMessage
- SingleStreamReadCommandMessage 
- MultiStreamReadCommandMessage 

The single CommandMessage classes could be removed by only using the multi command messages, but then we have to create a new StreamPostion array and a new StreamPostion object. I wasn't sure which option too choose, so feel free to give input on this (GC vs duplicate code).

There is also some code duplication between the Read and ReadGroup Command messages as the ReadGroup command is very similar to the XREAD but has some additional values in the front of the command (group, consumer, noack). A base class could be created as well to reduce some of the duplication (calculating the hashslot in case of multiple streams and writing the streams part of the commands).

The method CommandAndKey is also not overriden for these Messages as I wasn't which argument of the command to include in the string.

Co-authored-by: Niels Derdaele <niels.derdaele@alsic.be>
Co-authored-by: Nick Craver <nrcraver@gmail.com>
@NickCraver
Copy link
Collaborator

Fixed in follow-up PRs so closing out, thanks a ton for all the info and work here @nielsderdaele!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@NickCraver @nielsderdaele