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

Remove the NONDETERMINISTIC_OUTPUT flag from most CLUSTER sub-commands. #11157

Merged
merged 3 commits into from
Aug 28, 2022

Conversation

huangzhw
Copy link
Collaborator

@huangzhw huangzhw commented Aug 21, 2022

TLDR: the CLUSTER command originally had the random flag,
so all the sub-commands initially got that new flag, but in fact many
of them don't need it.
The only effect of this change is on the output of COMMAND INFO.

Original text:

cluster keyslot is deterministic.
I doubt whether other cluster commands is NONDETERMINISTIC_OUTPUT.
cluster myid
cluster flushslots

What definition of NONDETERMINISTIC_OUTPUT really is?

@oranagra
Copy link
Member

the definition is here: https://redis.io/docs/reference/command-tips/

i see that this flag was added to all these commands in #10104, i.e. in the initial PR that added this flag, and not some specific concern later realized, so chances are these are just an oversight. @guybe7 FYI.
considering some of these return a simple OK, it certainly doesn't make sense.
@huangzhw can you please go over them and fix the others?

@huangzhw
Copy link
Collaborator Author

Does different errors nondeterministic?
e.g. cluster flushslot may return "DB must be empty to perform CLUSTER FLUSHSLOTS.".
saveconfig may fail with "error saving the cluster node config: %s".
cluster forget may return different errors because of server role. If this is, role should also be.
cluster id I think it is deterministic.
So I think the definition is not enough.

@oranagra
Copy link
Member

I don't think any of these count as non-deterministic for that matter.
any command in redis may result with different response depending on state (such as WRONGTYPE, MASTERDOWN, LOADING, etc).
the non-deterministic flag is for commands like SPOP, RANDOMKEY, TTL, and even INFO, which could result in different response every time they're called, even on the exact same data set and state.

@huangzhw
Copy link
Collaborator Author

huangzhw commented Aug 23, 2022

Add more cluster commands.

@oranagra
Copy link
Member

@madolson can you please make sure we're not missing anything.
@guybe7 do you recall how come they all got this flag?

@guybe7
Copy link
Collaborator

guybe7 commented Aug 23, 2022

@oranagra before Redis 7 clusterCommand had the "random" flag, i think it just gave all subcommands NONDETERMINISTIC_OUTPUT, thinking to preserve the status quo (i don't know much about cluster commands so i preferred to leave it as-is rather than to do something wrong)

@madolson
Copy link
Contributor

Yeah, this list seems right. I might also include countkeysinslot, which is arguably deterministic based on the dataset, and dbsize doesn't have the flag.

@oranagra oranagra changed the title cluster keyslot is not NONDETERMINISTIC_OUTPUT. Remove the NONDETERMINISTIC_OUTPUT flag from most CLUSTER sub-commands. Aug 28, 2022
@oranagra oranagra merged commit a7da747 into redis:unstable Aug 28, 2022
@huangzhw huangzhw deleted the commands-tips branch August 28, 2022 09:57
oranagra pushed a commit to oranagra/redis that referenced this pull request Sep 21, 2022
…s. (redis#11157)

TLDR: the CLUSTER command originally had the `random` flag,
so all the sub-commands initially got that new flag, but in fact many
of them don't need it.
The only effect of this change is on the output of COMMAND INFO.

(cherry picked from commit a7da747)
oranagra pushed a commit that referenced this pull request Sep 21, 2022
…s. (#11157)

TLDR: the CLUSTER command originally had the `random` flag,
so all the sub-commands initially got that new flag, but in fact many
of them don't need it.
The only effect of this change is on the output of COMMAND INFO.

(cherry picked from commit a7da747)
Mixficsol pushed a commit to Mixficsol/redis that referenced this pull request Apr 12, 2023
…s. (redis#11157)

TLDR: the CLUSTER command originally had the `random` flag,
so all the sub-commands initially got that new flag, but in fact many
of them don't need it.
The only effect of this change is on the output of COMMAND INFO.
madolson pushed a commit to madolson/redis that referenced this pull request Apr 19, 2023
…s. (redis#11157)

TLDR: the CLUSTER command originally had the `random` flag,
so all the sub-commands initially got that new flag, but in fact many
of them don't need it.
The only effect of this change is on the output of COMMAND INFO.
enjoy-binbin pushed a commit to enjoy-binbin/redis that referenced this pull request Jul 31, 2023
…s. (redis#11157)

TLDR: the CLUSTER command originally had the `random` flag,
so all the sub-commands initially got that new flag, but in fact many
of them don't need it.
The only effect of this change is on the output of COMMAND INFO.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants