Implements [B]Z[REV]POP and the respective unit tests

[itamarhaber]

An implementation of the
Ze POP Redis Module as core
Redis commands.

Resolves #1861.

@antirez per our RedisConf discussion. Does it make sense IYO to put
effort into designing the zset equivalent of BRPOPLPUSH?

[itamarhaber]

Oh, also kind of touches #2801 (last point in the issue).

[itamarhaber]

Continuing the thoughts about possible additions to the API, it could be neat to have:

• [B]Z[REV]POPZADD - adds the popped element to a target zset. Score can be the original ('*'), +inf, -inf, some float decr before the first existing member ('^ n') or incr after last ('$ n'), or just an arbitrarily provided score.
• [B][L|R]POPZADD - similar to the above, but pops from a list instead of a zset.

Thoughts? Feedback? Comments?

[antirez]

Thanks @itamarhaber, @artix75 and I are reviewing this together right now, looks promising, we are going to fix a few things probably but I think the PR will made it into the core. News soon.

[itamarhaber]

Great! Looking fwd to your review :-)

…

On Fri, May 4, 2018, 18:23 Salvatore Sanfilippo ***@***.***> wrote: Thanks @itamarhaber <https://github.com/itamarhaber>, @artix75 <https://github.com/artix75> and I are reviewing this together right now, looks promising, we are going to fix a few things probably but I think the PR will made it into the core. News soon. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4879 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFx1_OZHUis1edPGYp5P8jxeEnaaxutrks5tvHKIgaJpZM4Tr_9E> .

[itamarhaber]

An additional thought: given streams' BLOCK subscommand, and in order to prevent API bloat, all the functionality could be refactored into a single ZPOP command.

[antirez]

Hello @itamarhaber, I merged the code inside my branch zpop, which is now pushed online. Your code was already excellent, so I only changed the things that wanted to change because of design, not implementation. My changes account to:

1. Naming. Now the commands are called [B]ZPOPMIN [B]ZPOPMAX. We have scores here, so min/max ideas are really trivial to memorize.
2. Semantic of the non blocking operations. Instead of getting multiple keys ZPOPMIN/MAX take an optional count argument. Blocking is useful to listen for multiple keys. Non blocking is much more useful to say the server "give me N items" IMHO.
3. Changed the "reverse" 0/1 values to "where" with the macros ZSET_MIN/ZSET_MAX.
4. Modified // comments into /* */ since we don't use the former inside Redis mostly.

That's all basically. Of course I modified the tests as well, but I've a few things I would like to see in tests or at least in "smoke testing" by hand to ensure everything is correct.

1. Tests should be modified to really use two encodings. It appears that your tests while they claim to use the two encodings, create the zset from scratch every time, so it's always a ziplist, however maybe I missed something, I was a bit in a rush.
2. I want to test at least visually that AOF/replication is ok. There should be no reasons for them to be wrong, but still it is pretty important to test manually this, and add in the random dataset creations a few of the new operations (the non blocking ones).
3. If we have them for lists, we should also add units for the blocking operations AOF / replication / ..., if we don't have, I can't ask, but the reality is that we should have all such tests 😭

So cool work. Before merging, would you like to review the code and comment on my changes?
Could you help me with the above activities? Do we want to fix commands.json, that also lacks everything about streams (we can split the work with me doing the streams stuff and you doing the ZPOP?).

Cheers

[itamarhaber]

Hello @antirez - I'm happy to get the news.

Your code was already excellent

TY, but FYI it was mostly an adaptation of the work you previously did on blocking list operations, so you should get some of the credit as well ;)

WRT to modifications:

1. Naming, a.k.a the other hardest problem. I'm totally good w/ the new names, the older were an attempt to keep inline with the "legacy". The "REV" notation came from ZREVRANGE, naturally, but min and max are much more intuitive.

2. Non blocking semantics. Agreed as well - following LPOP, a count makes more sense given users inputs than a multi-key pop, so omitting the key name makes perfect sense.

3. "reverese" to "where" Makes sense, more inline with code base. I actually remember deciding against using two new ZSET macros, but can't remember why... oh well.

4. /* Note to self: keep this in mind */

WRT tests:

1. "I was a bit in a rush" <- np. This is basically a ripoff from your existing tests on zsets and lists. The encoding, IIUC, is taken care of by running the tests twice (stressers), with different CONFIG SET ziplist-max* settings - 0 and the defaults.

2. I can definitely try that, but nothing will beat your ability to look at the Matrix code...

3. From my superficial understanding of the non-JimTcl test suite, I couldn't find an existing "integration" test for AOF/RDB/Replication and blocking commands, although the code "should" (famous last words) handle that. That said, with some guidance, I think I'm up for the challenge.

review the code and comment on my changes?

Yep - #4920

Could you help me with the above activities?

Will do a visual and give a shot at an integration test (again, any guidance will be appreciated). If I missed anything else - just lmk.

Do we want to fix commands.json, that also lacks everything about streams (we can split the work with me doing the streams stuff and you doing the ZPOP?)

Granted, now that the API is finalized I'll make the relevant PR. As for streams and other docs, lmk if you need help. UPDATED: PR at redis/redis-doc#922.

One last thing, any thoughts on BZPOP[MIN|MAX]ZADD? It could be a nice final touch (Z for Zygmuntowicz).

Cheers,

[Furgas]

Fantastic news. *ZADD variants will definitely make it as complete as it can possibly get.

[antirez]

Thanks @itamarhaber,

FYI it was mostly an adaptation of the work you previously did on blocking list operations

Code is the simple part, the point is to understand when to add a feautre! :-) After the refactoring of blocking operations needed to support Streams blocking stuff, to add BZPOP was much simpler, also after years we know that there are surely compelling reasons and patterns to have all this.

However I'm not for the BZPOPZADD part, because instead experience with lists shown that this is not a good idea in general, unfortunately, and that that adding safety of message processing may be used other means. The worst thing abut BZPOPZADD and BRPOPLPUSH and so forth are the cascading effects, they create a lot of problems in replication for instance, and our BRPOPLPUSH replication is still not correct in certain ways (we can talk about it if you want).

The encoding, IIUC, is taken care of by running the tests twice (stressers), with different CONFIG SET ziplist-max* settings - 0 and the defaults.

Oh right! Perfect so we are covered about that.

I couldn't find an existing "integration" test for AOF/RDB/Replication and blocking commands

Ok if we don't have for lists, for now I'll be happy enough to at least manual test all the code paths to see that the behavior is correct. I'll do it right now after writing this message and I'll report back.

UPDATED: PR at redis/redis-doc#922.

Thank you! More news about the propagation manual test soon.
But I'm thinking about also adding an integration test which is specific to blocking operations.

[antirez]

@itamarhaber quick update, the replication of blocking ZPOP was indeed not working. I fixed it but I'm doing more tests and I want to write an unit test at this point, doing a few random operations, checking at every step that the replicas are ok.

[itamarhaber]

Thanks for the update - more tests are usually a good idea 👍

[antirez]

Definitely... tests pushed, they are able to find the old replication bug, so apparently useful. Now everything is merged into unstable.

[itamarhaber]

So v5? Backport to 2.x? :-) woot.

[antirez]

Sure v5! Il mar 15 mag 2018, 17:54 Itamar Haber <notifications@github.com> ha scritto:

…

So v5? Backport to 2.x? :-) woot. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#4879 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEAYJs7nhTBKFPTaLy-9fdi-jScn6_Qks5tyvo5gaJpZM4Tr_9E> .