Add the support of the BZMPOP command #1490
Conversation
|
@Yangsx-1 You can use the |
Thanks, fixed. |
|
|
OK, i'll try, but it may need some time, i'm a little busy these days. |
It looks to be a false positive of ThreadSanitizer about TBB. Dont worry. |
Refactored. |
src/commands/cmd_zset.cc
Outdated
| return {Status::BlockingCmd}; | ||
| } | ||
|
|
||
| rocksdb::Status TryPopFromZset() { |
There was a problem hiding this comment.
Actually, the function pops from multiple sorted sets which is not expressed by its name. And, except for popping elements, it sends a reply. It's better to separate concerns: pop and return elements in one function and then analyze (if something was popped/if the mode is blocking/etc) and send the response in another function. Let the function do just one thing.
There was a problem hiding this comment.
Looks good to me with a glance. See if @torwig has more detailed review : )
|
@torwig Is there anything i need to modify? :) |
|
@Yangsx-1 Right now I don't know :) I'll have a look at the code later. |
|
@Yangsx-1 I made a PR with my changes to your branch. You can approve it and merge so they should be visible here. |
Thanks for your patience and modification! |
|
Thanks all, merging... |
|
Thanks for @Yangsx-1 contribution, you can add the command in https://github.com/apache/incubator-kvrocks-website/blob/main/docs/supported-commands.md if you like. Also thanks for the great review comments from @torwig and @PragmaTwice. |
We did not check for repeated parameters before, so the following code is executable: ``` 127.0.0.1:6666> zmpop 1 zset min max (nil) 127.0.0.1:6666> zmpop 1 zset min count 10 count 100 (nil) 127.0.0.1:6666> zmpop 1 zset min min max max min (nil) 127.0.0.1:6666> zmpop 1 zset min count 10 count 100 max (nil) 127.0.0.1:6666> bzmpop 0.1 1 zset min max (nil) 127.0.0.1:6666> bzmpop 0.1 1 zset min count 1 count 10 (nil) ``` Now we don't allow duplicate parameters (throw a syntax error): ``` 127.0.0.1:6666> zmpop 1 zset min max (error) ERR syntax error 127.0.0.1:6666> zmpop 1 zset min count 10 count 100 (error) ERR syntax error 127.0.0.1:6666> zmpop 1 zset min min max max min (error) ERR syntax error 127.0.0.1:6666> zmpop 1 zset min count 10 count 100 max (error) ERR syntax error 127.0.0.1:6666> bzmpop 0.1 1 zset min max (error) ERR syntax error 127.0.0.1:6666> bzmpop 0.1 1 zset min count 1 count 10 (error) ERR syntax error ``` Also added some tests to cover these wrong error paths. Refs: ZMPOP was added in apache#1468 and BZMPOP was added in apache#1490.
Close #1445
This PR add supports of bzmpop, bzpopmax, bzpopmin command like Redis.
And I also add some tests for these three commands.
(It's my first PR for kvrocks, there maybe something i didn't recognize. I'm very glad to revise it if there is something wrong.)