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

use the new key resolution algorithm #328

Merged
merged 4 commits into from
May 19, 2022
Merged

Conversation

pmlopes
Copy link
Member

@pmlopes pmlopes commented May 10, 2022

Signed-off-by: Paulo Lopes pmlopes@gmail.com

Motivation:

Redis 7 introduced a new way to perform key resolution. This PR removes the wrong way we were using and follows the requirements by redis itself.

Signed-off-by: Paulo Lopes <pmlopes@gmail.com>
Signed-off-by: Paulo Lopes <pmlopes@gmail.com>
Signed-off-by: Paulo Lopes <pmlopes@gmail.com>
@pmlopes
Copy link
Member Author

pmlopes commented May 10, 2022

@pendula95 redis 7 introduced metadata for the commands with an algorithm on how to extract the right keys from a command in order to compute the right cluster slot.

Before we had some basic heuristics and some ad-hoc key extractor that worked mostly.

This PR tries to remove the non-standard ways of finding if a cluster command is read only, keys and splitting.

All reviews are welcome :)

@pmlopes pmlopes added this to the 4.3.1 milestone May 10, 2022
Signed-off-by: Paulo Lopes <pmlopes@gmail.com>
@pmlopes pmlopes merged commit c5318fd into master May 19, 2022
@liuchong
Copy link
Contributor

Hi, what is the needGetKeys for? @pmlopes Thanks!

@liuchong
Copy link
Contributor

Another question, look at the below commands, are they readOnly or not?

Command CONFIG = new CommandImpl("config", -2, null, false, false);
Command SAVE = new CommandImpl("save", 1, null, false, false);

They are having a null readOnly argument by with no keyLocators, and they should by default got a readOnly true.

@pmlopes
Copy link
Member Author

pmlopes commented May 27, 2022

That information is extracted from the COMMANDS command. If a command has a flag read or write then it's clear if the command is read only or not, but some cases like these don't include any flag, so nothing can be said. Redis states that for these we should rely on the server to decide.

It's not a bug, it's what the metadata tells us. Before we were assuming all commands were either read only or not but it seems our assumption was wrong.

Do not you don't need this metadata unless you want to use a cluster connection. For most cases all you need is the regular command name.

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

Successfully merging this pull request may close these issues.

2 participants