Add ZMPOP/BZMPOP commands. by enjoy-binbin · Pull Request #9484 · redis/redis

enjoy-binbin · 2021-09-10T09:41:00Z

This is similar to the recent addition of LMPOP/BLMPOP (#9373), but zset.

Syntax for the new ZMPOP command:
ZMPOP numkeys [<key> ...] MIN|MAX [COUNT count]

Syntax for the new BZMPOP command:
BZMPOP timeout numkeys [<key> ...] MIN|MAX [COUNT count]

Some background:

ZPOPMIN/ZPOPMAX take only one key, and can return multiple elements.
BZPOPMIN/BZPOPMAX take multiple keys, but return only one element from just one key.
ZMPOP/BZMPOP can take multiple keys, and can return multiple elements from just one key.

Note that ZMPOP/BZMPOP can take multiple keys, it eventually operates on just on key.
And it will propagate as ZPOPMIN or ZPOPMAX with the COUNT option.

As new commands, if we can not pop any elements, the response like:

ZMPOP: Return a NIL in both RESP2 and RESP3, unlike ZPOPMIN/ZPOPMAX return emptyarray.
BZMPOP: Return a NIL in both RESP2 and RESP3 when timeout is reached, like BZPOPMIN/BZPOPMAX.

For the normal response is nested arrays in RESP2 and RESP3:

ZMPOP/BZMPOP
1) keyname
2) 1) 1) member1
      2) score1
   2) 1) member2
      2) score2

In RESP2:
1) "myzset"
2) 1) 1) "three"
      2) "3"
   2) 1) "two"
      2) "2"

In RESP3:
1) "myzset"
2) 1) 1) "three"
      2) (double) 3
   2) 1) "two"
      2) (double) 2

This is similar to the previous LMPOP/BLMPOP, except for zset. Syntax for the new ZMPOP command: `ZMPOP numkeys [<key> ...] MIN|MAX [COUNT count]` Syntax for the new BZMPOP command: `BZMPOP timeout numkeys [<key> ...] MIN|MAX [COUNT count]` Some background: - ZPOPMIN/ZPOPMAX take only one key, and can return multiple elements. - BZPOPMIN/BZPOPMAX take multiple keys, but return only one element from just one key. - ZMPOP/BZMPOP can take multiple keys, and can return multiple elements from just one key. Note that ZMPOP/BZMPOP can take multiple keys, it eventually operates on just on key. And it will propagate as ZPOPMIN or ZPOPMAX with the COUNT option. As new commands, if we can not pop any elements, the response is the same as before: - ZMPOP: Return an emptyarray in both RESP2 and RESP3, like ZPOPMIN/ZPOPMAX. - BZMPOP: Return a NIL in both RESP2 and RESP3 when timeout is reached, like BZPOPMIN/BZPOPMAX. For the normal response is nested arrays in RESP2 and RESP3: ``` ZMPOP/BZMPOP 1) keyname 2) 1) 1) member1 2) score1 2) 1) member2 2) score2 In RESP2: 1) "myzset" 2) 1) 1) "three" 2) "3" 2) 1) "two" 2) "2" In RESP3: 1) "myzset" 2) 1) 1) "three" 2) (double) 3 2) 1) "two" 2) (double) 2 ```

oranagra · 2021-09-10T11:06:58Z

Some background on the response format.
It was discussed here: #8824 (comment)
And later in #9373

First, like ZPOPMIN we can't use a RESP3 map since the order matters.
Secondly, like the recent LMPOP we decided not to use a flat array in RESP2 (unlike ZPOPMIN, but like SCAN always did).
And although semantically we can return an array of triplets, repeating the key name for each pair, since we always pop only from one key, we decided to return the key name only once.

oranagra · 2021-09-12T08:42:03Z

As new commands, if we can not pop any elements, the response is the same as before:
ZMPOP: Return an emptyarray in both RESP2 and RESP3, like ZPOPMIN/ZPOPMAX.
BZMPOP: Return a NIL in both RESP2 and RESP3 when timeout is reached, like BZPOPMIN/BZPOPMAX.

I'm not sure it's wise for ZMPOP to return an empty array when it can't pop, i think it should return NIL like the new LMPOP.
In the case of the existing ZPOP, these return an array of 0 or more pairs, so maybe an empty array is ok.
but for ZMPOP, it returns a stricture containing key name and a list of pair, so i think this makes it more similar to BZPOP and LMPOP. i.e the response is an invalid one, not an empty list.
@itamarhaber WDYT?

oranagra

looks good. obviously missing tests..

src/blocked.c

src/db.c

src/t_zset.c

Co-authored-by: Oran Agra <[email protected]>

oranagra

changes LGTM

src/t_zset.c

madolson

API looks good, this looks like it still needs tests as Oran mentioned.

src/server.c

enjoy-binbin · 2021-09-18T06:10:22Z

Look like i added a bunch of test code again...

tests/integration/aof.tcl

tests/unit/type/zset.tcl

Co-authored-by: Oran Agra <[email protected]>

tests/unit/type/list.tcl

oranagra

@redis/core-team please approve the new commands.
@enjoy-binbin please make a redis-doc PR

oranagra · 2021-09-19T14:30:18Z

triggered a partial CI on all platforms: https://github.com/redis/redis/actions/runs/1250894823
valgrind and all platforms passed: (--single integration/aof --single unit/type/list --single unit/type/zset)

The previous code did not check whether COUNT is set. So we can use `lmpop 2 key1 key2 left count 1 count 2`. This situation can occur in LMPOP/BLMPOP/ZMPOP/BZMPOP commands. LMPOP/BLMPOP introduced in redis#9373, ZMPOP/BZMPOP introduced in redis#9484.

The previous code did not check whether COUNT is set. So we can use `lmpop 2 key1 key2 left count 1 count 2`. This situation can occur in LMPOP/BLMPOP/ZMPOP/BZMPOP commands. LMPOP/BLMPOP introduced in #9373, ZMPOP/BZMPOP introduced in #9484.

This bug was introduced in redis#9484 (7.0.0). It result that BZMPOP blocked on non-key arguments. Like `bzmpop 0 1 myzset min count 10`, this command will additionally block in these keys (except for the first and the last argument): - 0: timeout value - 1: numkeys value - min: min/max token - count: count token Fixes redis#10762

This bug was introduced in #9484 (7.0.0). It result that BZMPOP blocked on non-key arguments. Like `bzmpop 0 1 myzset min count 10`, this command will additionally block in these keys (except for the first and the last argument) and can return their values: - 0: timeout value - 1: numkeys value - min: min/max token - count: count token

This bug was introduced in redis#9484 (7.0.0). It result that BZMPOP blocked on non-key arguments. Like `bzmpop 0 1 myzset min count 10`, this command will additionally block in these keys (except for the first and the last argument) and can return their values: - 0: timeout value - 1: numkeys value - min: min/max token - count: count token

oranagra reviewed Sep 12, 2021

View reviewed changes

src/t_zset.c Show resolved Hide resolved

oranagra added release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus state:needs-doc-pr requires a PR to redis-doc repository state:needs-test-code the PR is missing test code labels Sep 12, 2021

code review from oran

9cf80ea

Co-authored-by: Oran Agra <[email protected]>

oranagra reviewed Sep 13, 2021

View reviewed changes

src/t_zset.c Outdated Show resolved Hide resolved

enjoy-binbin added 2 commits September 14, 2021 11:05

CR: add reply_nil_when_empty flag and revert some link break

97e8cc0

ZMPOP always replicate it as ZPOP[MIN|MAX]

d92c9e8

madolson reviewed Sep 17, 2021

View reviewed changes

src/server.c Outdated Show resolved Hide resolved

enjoy-binbin added 3 commits September 18, 2021 13:17

add test code

703cfb8

Merge remote-tracking branch 'upstream/unstable' into add_zmpop_bzmpop2

ea3b9fc

update key spec

a6874cf

oranagra reviewed Sep 18, 2021

View reviewed changes

enjoy-binbin and others added 2 commits September 19, 2021 19:06

code review from oran

6352f52

Co-authored-by: Oran Agra <[email protected]>

lazy me..

a60e66b

oranagra reviewed Sep 19, 2021

View reviewed changes

tests/unit/type/list.tcl Outdated Show resolved Hide resolved

tests/unit/type/list.tcl Outdated Show resolved Hide resolved

enjoy-binbin added 3 commits September 19, 2021 20:21

code review

80c7067

just wait for a nil response

391dae1

rd -> r

83f09c7

oranagra approved these changes Sep 19, 2021

View reviewed changes

oranagra added approval-needed Waiting for core team approval to be merged 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 and removed state:needs-test-code the PR is missing test code labels Sep 19, 2021

itamarhaber approved these changes Sep 19, 2021

View reviewed changes

enjoy-binbin mentioned this pull request Sep 20, 2021

Add new ZMPOP and BZMPOP commands. redis/redis-doc#1654

Merged

yossigo approved these changes Sep 22, 2021

View reviewed changes

oranagra merged commit 14d6abd into redis:unstable Sep 23, 2021

enjoy-binbin deleted the add_zmpop_bzmpop branch September 23, 2021 06:04

enjoy-binbin mentioned this pull request Oct 31, 2021

Fix multiple COUNT in LMPOP/BLMPOP/ZMPOP/BZMPOP #9701

Merged

hwware mentioned this pull request Dec 23, 2021

Add new commands called lmmove & blmmove. #9929

Open

enjoy-binbin mentioned this pull request May 23, 2022

Fix BZMPOP gets unblocked by non-key args and returns them #10764

Merged

Conversation

enjoy-binbin commented Sep 10, 2021 • edited by oranagra Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oranagra commented Sep 10, 2021

Uh oh!

oranagra commented Sep 12, 2021

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

madolson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

enjoy-binbin commented Sep 18, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

oranagra commented Sep 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

enjoy-binbin commented Sep 10, 2021 •

edited by oranagra

Loading

oranagra commented Sep 19, 2021 •

edited

Loading