Add LMPOP/BLMPOP commands. #9373

enjoy-binbin · 2021-08-14T13:42:11Z

We want to add COUNT option for BLPOP.
But we can't do it without breaking compatibility due to the command arguments syntax.
So this commit introduce two new commands.

Syntax for the new LMPOP command:
LMPOP numkeys [<key> ...] LEFT|RIGHT [COUNT count]

Syntax for the new BLMPOP command:
BLMPOP timeout numkeys [<key> ...] LEFT|RIGHT [COUNT count]

Some background:

LPOP takes one key, and can return multiple elements.
BLPOP takes multiple keys, but returns one element from just one key.
LMPOP can take multiple keys and return multiple elements from just one key.

Note that LMPOP/BLMPOP can take multiple keys, it eventually operates on just one key.
And it will propagate as LPOP or RPOP with the COUNT option.

As a new command, it still return NIL if we can't pop any elements.
For the normal response is nested arrays in RESP2 and RESP3, like:

LMPOP/BLMPOP 
1) keyname
2) 1) element1
   2) element2

I.e. unlike BLPOP that returns a key name and one element so it uses a flat array,
and LPOP that returns multiple elements with no key name, and again uses a flat array,
this one has to return a nested array, and it does for for both RESP2 and RESP3 (like SCAN does)

Some discuss can see: #766 #8824

We want to add COUNT option for BLPOP. But we can't do it without breaking compatibility. So this commit introduce a new command. Syntax for the new LMPOP command: LMPOP numkeys [<key> ...] LEFT|RIGHT [COUNT count] [BLOCK timeout] Some background: - LPOP takes one key, and can return multiple elements. - BLPOP takes multiple keys, but returns one element from just one key. - [B]LMPOP can take multiple keys and return multiple elements from just one key. Note that [B]LMPOP can take multiple keys, it eventually operates on just one key. And it will propagate as LPOP or RPOP with the COUNT option. As a new command, it still return NIL if we can't pop any elements. For the normal response is nested arrays in RESP2 and RESP3, like: [B]LMPOP 1) keyname 2) 1) element1 2) element2

enjoy-binbin · 2021-08-14T13:46:08Z

Some usage:

127.0.0.1:6379> lmpop 2 non1 non2 right count 10
(nil)
127.0.0.1:6379> lpush mylist a b c
(integer) 3
127.0.0.1:6379> lmpop 1 mylist left
1) "mylist"
2) 1) "c"
127.0.0.1:6379> lmpop 1 mylist right count 10
1) "mylist"
2) 1) "a"
   2) "b"
127.0.0.1:6379> lpush mylist a b c
(integer) 3
127.0.0.1:6379> lpush mylist2 1 2 3
(integer) 3
127.0.0.1:6379> lmpop 2 mylist mylist2 right count 10
1) "mylist"
2) 1) "a"
   2) "b"
   3) "c"
127.0.0.1:6379> lmpop 2 mylist mylist2 right count 10
1) "mylist2"
2) 1) "1"
   2) "2"
   3) "3"

with block:

127.0.0.1:6379> blmpop 1 2 mylist mylist2 right count 10
(nil)
(1.04s)
# other client do a lpush mylist a b c
127.0.0.1:6379> blmpop 0 2 mylist mylist2 right count 10
1) "mylist"
2) 1) "a"
   2) "b"
   3) "c"
(12.85s)
# other client do a lpush mylist2 a
127.0.0.1:6379> blmpop 0 2 mylist mylist2 right count 10
1) "mylist2"
2) 1) "a"
(18.53s)

oranagra

gave you some comments,
the most important one is about separating reply, popping and deleting roles properly between common code with other parts of redis.

i haven't reviewed the tests yet. i see there are quite a lot of them, can you please describe what you did to create them? i.e. did you just copy all the existing tests for BLPOP or something of that sort? any advise of how to review that?

src/blocked.c

src/server.h

src/t_list.c

src/server.c

src/t_list.c

madolson · 2021-08-17T06:30:19Z

I think we had a preference for having separate blocking commands based on feedback from client developers. Just making a note here.

I'd also prefer:

LMPOP LEFT|RIGHT COUNT key1 ...

The syntax of having num keys is sort of awkward for clients to do.

guybe7 · 2021-08-17T07:37:53Z

@madolson if all the keys are in the end (without any keyword) we can't have optional arguments (it's gonna be hard for clients to detect)
we should either have a constant index where keys start or use keynum/keyword scheme

madolson · 2021-08-17T07:41:43Z

I think having a "keys" keyword works too.

guybe7 · 2021-08-17T07:42:51Z

clients prefer some keyword at unknown index rather than keynum at a fixed index?

guybe7 · 2021-08-17T07:44:02Z

for the record: i also prefer having a separate blocking command rather than BLOCK

oranagra · 2021-08-17T07:47:44Z

@enjoy-binbin sorry for the back and forth, please make the changes to separate LMPOP and BLMPOP.

regarding the keyword vs keynum discussion, even if we do with with keyword, we'll still need a keynum argument, i think a keynum at a fixed argument, like we have for EVAL and ZUNION is fine.

enjoy-binbin · 2021-08-17T07:57:10Z

Sure. no problems. I will take care of the LMPOP/BLMPOP and other suggestions from the review.
So we will go this way?

LMPOP numkeys [<key> ...] LEFT|RIGHT [COUNT count]
BLMPOP numkeys [<key> ...] LEFT|RIGHT [COUNT count] TIMEOUT

agree madolson's comment below

madolson · 2021-08-17T07:57:15Z

It seemed more user friendly to not have to compute the length, but it's not something I feel strongly about. We should also probably write down our command structure best practices though, being consistent is more important than what we pick.

oranagra · 2021-08-17T08:00:22Z

It seemed more user friendly to not have to compute the length

@madolson what do you mean?

madolson · 2021-08-17T21:14:44Z

@oranagra If I'm doing it on the cli manually:

redis-cli LMPOP LEFT COUNT 50 KEYS key1 key2 key3 key4

Is slightly easier than

redis-cli LMPOP 4 key1 key2 key3 key4 LEFT COUNT 50

It's much easier to get the count wrong and have a non-obvious error. But I also acknowledge that from an internal implementation perspective, having the fixed position number of arguments is also nice. Also, most clients will abstract this away.

oranagra · 2021-08-18T06:11:12Z

@madolson in your first example there's no key_count, do you mean that as soon as we hit the KEYS argument, the remainder of the arguments are keys?
IIIRC the only command that operates like that is the dreaded MIGRATE command.
@itamarhaber @guybe7 anything to add?

guybe7 · 2021-08-18T09:23:02Z

I think the pros of using "keynum" outweigh the cons...

1. separate LMPOP and BLMPOP 2. remove 'del' arg 3. pop inside serveClientBlockedOnList 4. other fix from review Co-authored-by: Oran Agra <oran@redislabs.com>

src/blocked.c

oranagra · 2021-08-24T19:18:00Z

looking at the new command syntax:

BLMPOP <numkeys> [<key> ...] LEFT|RIGHT [COUNT count] <timeout>

i think this last argument thing is what we have in BLPOP today, is bad.
i.e. BLPOP implicitly realizes the number of keys by argc-1.
but in this command we have the numkeys positional argument, we should either have the timeout a positional argument (i.e. maybe before the numkeys), or add a modifier argument.
i.e. one of these:

BLMPOP <timeout> <numkeys> [<key> ...] LEFT|RIGHT [COUNT count]
BLMPOP <numkeys> [<key> ...] LEFT|RIGHT [COUNT count] [TIMEOUT <timeout>]

if we take the second option, default timeout could be 0, i.e. infinite.
@madolson @itamarhaber WDYT?

Co-authored-by: Oran Agra <oran@redislabs.com>

madolson · 2021-08-25T05:31:34Z

BLMPOP <timeout> <numkeys> [<key> ...] LEFT|RIGHT [COUNT count]

Minor preference for this one. In all other blocking commands the timeout is a required argument and I think that makes sense. I don't think timeout should be optional/default to 0. Even for XADD you can't do blocking with an implied default.

src/t_list.c

tests/integration/aof.tcl

tests/unit/type/list.tcl

oranagra · 2021-08-30T10:51:55Z

@enjoy-binbin please prepare a redis-doc PR

enjoy-binbin · 2021-08-30T16:41:29Z

Doc PR created at redis/redis-doc#1636

yoav-steinberg · 2021-08-31T09:11:13Z

Question: are these commands officially deprecating B[RL]POP and [RL]POP? Should we document them as such?

oranagra · 2021-09-02T10:36:35Z

i didn't intend to deprecate these, but maybe that't the logical thing to do.
we did deprecate GEORADIUS, but that's also because the interface had issues,
but we also deprecated BRPOPLPUSH and Salvatore deprecated HMSET, and these didn't have interface issues.
@yossigo @madolson @itamarhaber WDYT?

madolson · 2021-09-07T00:36:58Z

@oranagra I'm not in favor of deprecating them, the four essential list commands are [L|R]PUSH and [L|R]POP and they are easy to understand. I don't want to deprecate half of them for a new command that is both more complex and introduces more inconsistencies.

oranagra · 2021-09-07T11:05:37Z

@madolson i agree, but trying to find a logical justification to explain why we did that for some and don't do that here.
For GEORADIUS it's that we didn't like it (the store argument).
But what about RPOPLPUSH, HMGET, ZRANGEBYSCORE?

madolson · 2021-09-07T15:29:16Z

RPOPLPUSH was an outlier command that wasn't able to implement all use cases, so that one makes sense to deprecate.

I don't really know if I agree the other two should have been deprecated. Having a single ZRANGE is much nicer than a half dozen variants though.

oranagra · 2021-09-07T15:45:45Z

Arguably LPOP and BLPOP are also not "able to implement all use cases" (I.E popping multiple elements or from multiple keys). But ok, it's just a Metadata flag, it doesn't really do anything, and we can always add it later.
Also, there's really no harm in supporting both (the old commands and the new commands), even if just for users convenience.
So let's not mark them as depreciated now.

yoav-steinberg · 2021-09-09T06:25:38Z

So let's not mark them as depreciated now.

Very well. I'll go over the docs again in light of this.

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 ```

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.

tests/unit/type/list.tcl

In redis#9373, actually need to replace `$rd $pop blist1{t} blist2{t} 1` with `bpop_command_two_key $rd $pop blist1{t} blist2{t} 1` but forgot to delete the latter. This doesn't affect the test, because the later assert_error "WRONGTYPE" is expected (and right). And if we read $rd again, it will get the wrong result, like 'ERR unknown command 'BLMPOP_LEFT' | 'BLMPOP_RIGHT'

In #9373, actually need to replace `$rd $pop blist1{t} blist2{t} 1` with `bpop_command_two_key $rd $pop blist1{t} blist2{t} 1` but forgot to delete the latter. This doesn't affect the test, because the later assert_error "WRONGTYPE" is expected (and right). And if we read $rd again, it will get the wrong result, like 'ERR unknown command 'BLMPOP_LEFT' | 'BLMPOP_RIGHT'

In redis#9373, actually need to replace `$rd $pop blist1{t} blist2{t} 1` with `bpop_command_two_key $rd $pop blist1{t} blist2{t} 1` but forgot to delete the latter. This doesn't affect the test, because the later assert_error "WRONGTYPE" is expected (and right). And if we read $rd again, it will get the wrong result, like 'ERR unknown command 'BLMPOP_LEFT' | 'BLMPOP_RIGHT'

oranagra added release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus labels Aug 15, 2021

oranagra reviewed Aug 16, 2021

View reviewed changes

code review

5f960bd

1. separate LMPOP and BLMPOP 2. remove 'del' arg 3. pop inside serveClientBlockedOnList 4. other fix from review Co-authored-by: Oran Agra <oran@redislabs.com>

oranagra reviewed Aug 24, 2021

View reviewed changes

src/blocked.c Outdated Show resolved Hide resolved

for test

55011ef

enjoy-binbin and others added 2 commits August 25, 2021 13:17

CR: Set a deleted flag indicated whether the list is deleted.

1c02fea

Co-authored-by: Oran Agra <oran@redislabs.com>

code clean up

ed13f32

oranagra reviewed Aug 25, 2021

View reviewed changes

src/t_list.c Outdated Show resolved Hide resolved

src/t_list.c Outdated Show resolved Hide resolved

enjoy-binbin added 2 commits August 25, 2021 15:43

CR: Put output var on the last and cleanup.

44462a3

update comment about the deleted

302352d

enjoy-binbin force-pushed the lmpop_command branch from 1b55f8d to 302352d Compare August 25, 2021 08:03

adjust the test code for easy review

b4f3f06

oranagra reviewed Aug 29, 2021

View reviewed changes

oranagra linked an issue Aug 30, 2021 that may be closed by this pull request

BLPOP and BRPOP with COUNT #766

Closed

yossigo approved these changes Aug 30, 2021

View reviewed changes

madolson approved these changes Aug 30, 2021

View reviewed changes

enjoy-binbin added 2 commits September 1, 2021 13:19

Missing a decrRefCount

f28800f

Fix some minor phrasing issues

8fabda1

Missing a decrRefCount...

d2e21c4

oranagra merged commit c50af0a into redis:unstable Sep 9, 2021

enjoy-binbin deleted the lmpop_command branch September 9, 2021 09:21

oranagra mentioned this pull request Sep 10, 2021

Add ZMPOP/BZMPOP commands. #9484

Merged

mp911de mentioned this pull request Oct 29, 2021

Add support for LMPOP and BLMPOP commands redis/lettuce#1903

Closed

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

guybe7 reviewed Feb 14, 2023

View reviewed changes

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

enjoy-binbin mentioned this pull request Feb 15, 2023

Remove wrong code in list pop test #11805

Merged

Add LMPOP/BLMPOP commands. #9373

Add LMPOP/BLMPOP commands. #9373

Uh oh!

Conversation

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

Uh oh!

Uh oh!

enjoy-binbin commented Aug 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Uh oh!

madolson commented Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guybe7 commented Aug 17, 2021

Uh oh!

madolson commented Aug 17, 2021

Uh oh!

guybe7 commented Aug 17, 2021

Uh oh!

guybe7 commented Aug 17, 2021

Uh oh!

oranagra commented Aug 17, 2021

Uh oh!

enjoy-binbin commented Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

madolson commented Aug 17, 2021

Uh oh!

oranagra commented Aug 17, 2021

Uh oh!

madolson commented Aug 17, 2021 • edited by oranagra Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oranagra commented Aug 18, 2021

Uh oh!

guybe7 commented Aug 18, 2021

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

madolson commented Aug 25, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oranagra commented Aug 30, 2021

Uh oh!

enjoy-binbin commented Aug 30, 2021

Uh oh!

yoav-steinberg commented Aug 31, 2021

Uh oh!

oranagra commented Sep 2, 2021

Uh oh!

madolson commented Sep 7, 2021

Uh oh!

oranagra commented Sep 7, 2021

Uh oh!

madolson commented Sep 7, 2021

Uh oh!

oranagra commented Sep 7, 2021

Uh oh!

enjoy-binbin commented Aug 14, 2021 •

edited by oranagra

Loading

enjoy-binbin commented Aug 14, 2021 •

edited

Loading

madolson commented Aug 17, 2021 •

edited

Loading

enjoy-binbin commented Aug 17, 2021 •

edited

Loading

madolson commented Aug 17, 2021 •

edited by oranagra

Loading

oranagra commented Aug 24, 2021 •

edited

Loading