Skip to content

Change return value type for ZPOPMAX/MIN in RESP3#8981

Merged
oranagra merged 2 commits intoredis:unstablefrom
jhelbaum:fix_8824_zrange_return_type
Jun 16, 2021
Merged

Change return value type for ZPOPMAX/MIN in RESP3#8981
oranagra merged 2 commits intoredis:unstablefrom
jhelbaum:fix_8824_zrange_return_type

Conversation

@jhelbaum
Copy link
Contributor

@jhelbaum jhelbaum commented May 23, 2021

When using RESP3, ZPOPMAX/ZPOPMIN should return nested arrays for consistency
with other commands (e.g. ZRANGE).

We do that only when COUNT argument is present (similarly to how LPOP behaves).
for reasoning see #8824 (comment)

This is a breaking change only when RESP3 is used, and COUNT argument is present!

@oranagra oranagra linked an issue May 24, 2021 that may be closed by this pull request
@oranagra oranagra added release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus labels May 24, 2021
@jhelbaum jhelbaum force-pushed the fix_8824_zrange_return_type branch from 14590fa to 90f5a8d Compare June 12, 2021 23:31
jhelbaum added 2 commits June 13, 2021 15:21
…PMIN in RESP3

When using RESP3, ZPOPMAX/ZPOPMIN should return nested arrays for consistency
with other commands.
@jhelbaum jhelbaum force-pushed the fix_8824_zrange_return_type branch from 9d1bf41 to 69ce548 Compare June 13, 2021 12:21
@oranagra oranagra added the approval-needed Waiting for core team approval to be merged label Jun 13, 2021
@oranagra oranagra added the state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten label Jun 14, 2021
@oranagra oranagra changed the title Bugfix: Issue #8824 - confused return value type for ZPOPMAX/MIN in RESP3 Change return value type for ZPOPMAX/MIN in RESP3 Jun 16, 2021
@oranagra oranagra merged commit 7f34202 into redis:unstable Jun 16, 2021
@oranagra oranagra added the breaking-change This change can potentially break existing application label Jul 20, 2021
@oranagra
Copy link
Member

we decided to backport this to 6.0 and 6.2 mainly because RESP3 isn't yet heavily used, so the chance this will do some breakage is low, and on the other hand if some new app will start using resp3 on a recent release of 6.0 / 6.2, we prefer that i'll have that change from the get go.

@jhelbaum jhelbaum deleted the fix_8824_zrange_return_type branch July 20, 2021 10:21
This was referenced Jul 21, 2021
oranagra pushed a commit that referenced this pull request Jul 21, 2021
When using RESP3, ZPOPMAX/ZPOPMIN should return nested arrays for consistency
with other commands (e.g. ZRANGE).

We do that only when COUNT argument is present (similarly to how LPOP behaves).
for reasoning see #8824 (comment)

This is a breaking change only when RESP3 is used, and COUNT argument is present!

(cherry picked from commit 7f34202)
(cherry picked from commit caaad2d)
oranagra pushed a commit that referenced this pull request Jul 21, 2021
When using RESP3, ZPOPMAX/ZPOPMIN should return nested arrays for consistency
with other commands (e.g. ZRANGE).

We do that only when COUNT argument is present (similarly to how LPOP behaves).
for reasoning see #8824 (comment)

This is a breaking change only when RESP3 is used, and COUNT argument is present!

(cherry picked from commit 7f34202)
JackieXie168 pushed a commit to JackieXie168/redis that referenced this pull request Sep 8, 2021
When using RESP3, ZPOPMAX/ZPOPMIN should return nested arrays for consistency
with other commands (e.g. ZRANGE).

We do that only when COUNT argument is present (similarly to how LPOP behaves).
for reasoning see redis#8824 (comment)

This is a breaking change only when RESP3 is used, and COUNT argument is present!
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 breaking-change This change can potentially break existing application release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten

Projects

Development

Successfully merging this pull request may close these issues.

confused return value type(ZPOPMAX/ZPOPMIN--ZRANGE)

4 participants