Skip to content

Support by/get options for sort(_ro) in cluster mode when pattern implies slot.#12728

Merged
oranagra merged 4 commits intoredis:unstablefrom
CharlesChen888:cluster-support-sort-byget-hashtag
Dec 13, 2023
Merged

Support by/get options for sort(_ro) in cluster mode when pattern implies slot.#12728
oranagra merged 4 commits intoredis:unstablefrom
CharlesChen888:cluster-support-sort-byget-hashtag

Conversation

@CharlesChen888
Copy link
Contributor

@CharlesChen888 CharlesChen888 commented Nov 6, 2023

The by/get options of sort/sort_ro command used to be forbidden in cluster mode, since we are not sure which slot the pattern may be in.

As the optimization done in #12536, patterns now can be mapped to slots, we should allow by/get options in cluster mode when the pattern maps to the same slot as the key.

Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. This is a semantic change, a different reply to the client in some cases. The SCAN PR was just an optimization, i.e. the same reply to the client as before.

This needs to be discussed by the core team.

@CharlesChen888 CharlesChen888 force-pushed the cluster-support-sort-byget-hashtag branch from c291c6e to 412c199 Compare November 6, 2023 11:18
@soloestoy soloestoy added the release-notes indication that this issue needs to be mentioned in the release notes label Nov 16, 2023
@oranagra oranagra added state:major-decision Requires core team consensus approval-needed Waiting for core team approval to be merged labels Nov 16, 2023
Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. needs core-team approval

@oranagra
Copy link
Member

@redis/core-team as discussed in the meeting, please approve this one.

@soloestoy
Copy link
Contributor

@oranagra I see the keys optimization merged, can this PR also be merged now?

@oranagra
Copy link
Member

oranagra commented Dec 6, 2023

@yossigo @itamarhaber please approve

@oranagra oranagra merged commit e95a5d4 into redis:unstable Dec 13, 2023
zuiderkwast added a commit to redis/redis-doc that referenced this pull request Jan 18, 2024
Adding to the cluster spec documentation of the following Redis features:

redis/redis#12754 (SCAN)
redis/redis#12536 (KEYS)
redis/redis#12728 (SORT, SORT_RO)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval-needed Waiting for core team approval to be merged release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

8 participants