-
Notifications
You must be signed in to change notification settings - Fork 23.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds new pop-push commands (LMOVE, BLMOVE) #6929
Conversation
I started implementing this almost 5 years ago, so I decided to create a new PR. The old one is here: #2664 |
I was in doubt about mainly 3 things:
|
+1 for being persistent :) |
These new commands are symmetrical to the rpoplpush command, and they've been implemented using a generic method (poppushGenericCommand) with the existing pattern of a flag begin LIST_HEAD or LIST_TAIL to indicate the start or end of a list. The blocking equivalents will come in the following commit.
These new commands are symmetrical to the brpoplpush command, and they've been implemented using a generic method (blockingPopPushGenericCommand) with the existing pattern of a flag begin LIST_HEAD or LIST_TAIL to indicate the start or end of a list. The non-blocking alternatives were implemented in the previous commit.
Did a rebase with unstable solving a minor conflict (wasn't an actual conflict, just a change very close to my changes flagged as a possible conflict). As a plus it solved the CI checks that were failing before (and wasn't my fault as expected). |
Looks like a good coincise implementation @felipou, thanks. And since this is a plain extension of the original implementation, I guess the replication part should work as expected (didn't check the details). So indeed it is worth a careful review and a potential merge. |
Great, let me know if there is anything else I can do to help or improve the code! |
@yossigo @oranagra @madolson @soloestoy please review and consider for 6.0.6 @felipou would it be too much to ask you to help with the respective docs once approved? ;) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@felipou thank you for this PR.
i've added a few minor comments about the code.
some suggestions about improving some tests, and trimming others.
Not at all, please count me in for anything you need, it's a pleasure contributing to Redis! |
@oranagra Thanks for your review, I'll go over everything right now. I may struggle a bit with the tests, since I've never worked with tcl, I just copied the existing tests for RPOPLPUSH, but I think I'll manage. Just a quick question: what is the usual process here, do I overwrite my branch ( |
@felipou i like my git history clean too, we'll either squash it at the end, or if for some reason we want to keep some changes in separate commit, we'll do some cleanup. if you need help with the tests let me know, i think you'll just be able to use your clipboard and common sense.. |
- Replace "after 1000" with "wait_for_condition" when wait for clients to block/unblock. - Add a pre-existing element to target list on basic tests so that we can check if the new element was added to the correct side of the list. - Changed second call on linked poppush call test (instead of two brpoplpush, we do first a brpoplpush and then blpoprpush). Changes suggested by @oranagra during PR review.
Rename two functions, remove some empty lines and add some comments.
@oranagra I just pushed some new commits addressing everything you commented. I just replied every thread instead of resolving the conversations, if you want I can resolve all the trivial ones (removing duplicate tests for example). |
@felipou thanks you. everything looks great. p.s. out of curiosity i run the list test. |
@oranagra Great, thanks! Makes total sense that it runs faster too, the idea you gave of replacing the |
@redis/core-team since this PR adds commands (which we can't later remove or rename), it requires more than lazy consensus. |
I think the new commands are sensible. I want to bring up a separate point that I think we should rewrite the blocking command infrastructure. Today it works by marking the client as blocked on a key with a bunch of special variables, when that key is written to we used those stored special variables to complete the request. This has several issues:
Instead, I think we should merge the blocking framework with the mechanism that was outlined for the background threads. If a client is block from a non-existent key, it should just remove itself from the event handler and register that it is blocked on a key. When that key is touched, the blocked client will be unblocked and will attempt to re-execute the command that blocked it in the first place. This shouldn't introduce any major compatibility issues and should throw the same exceptions we do today. It can then inline the command it has within the replication system. It should hopefully remove a lot of the weird checks that we have to do that are included in the CR. I think this refactor is worth it either for this change or so that it is easier to make these sort of changes in the future. (If we agree it's worth it for this change, I'm okay with this specific PR getting merged and a separate one for the refactor) |
Instead of using rewriteClientCommandVector to transform rpoplpush/brpoplpush commands into lmove/blmove commands, we created generic functions that both commands use. At the same time, we have changed the rewrites and propagations to maintain compatibility for the rpoplpush/brpoplpush commands.
Just pushed 3 commits with:
I'll now experiment with the wherefrom/whereto parameters of the blockForKeys function, and update the redis-doc PR. |
Option 1 - using LIST_NONE: felipou@073bb77 Not sure if it was just me choosing terrible names, but option 2 doesn't look very good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all looks good. the only thing that may be missing is maybe add a test to cover the propagation of BRPOPLPUSH and BLMOVE to AOF or Replicas.
i see replication.tcl has these two tests:
test {BRPOPLPUSH replication, when blocking against empty list} {
test {BRPOPLPUSH replication, list exists} {
i guess these can be improved to look at the command stats to check which command was executed on the replica.
and add similar tests for BLMOVE.
maybe move proc cmdstat
from integration/introspection-2.tcl to support/util.tcl and use it in replication.tcl
Just pushed the commit to create the internal listPos struct as you suggested @oranagra, looks much better now, thanks! |
I'll take a look at the tests you mentioned. |
Ok, I think I managed. TCL is really new to me, so I didn't know much about what was possible. I created a new proc to avoid changing the file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hope my changes actually work (not a TCL expert either)
Change the redis client used to check for the cmd stats in the replica client. Also change the name of the commands searched for in the stats to be the non-blocking version.
Ok, great, thanks for the orientation! This actually helped me understand something I had suspected but wasn't really sure: the propagated command is actually the non-blocking version (I had to fix that for the tests to pass). That's how it worked in the rpoplpush command before my changes, so I just replicated the same idea (BLMOVE propagates LMOVE). Should we change that? |
@felipou change what? |
I thought maybe this wasn't the intended behaviour, and that perhaps we should propagate the same command (the blocking one) instead. |
@redis/core-team please approve the addition of new LMOVE and BLMOVE commands (deprecating [B]RPOPLPUSH). Note that when receiving a BRPOPLPUSH we'll still propagate an RPOPLPUSH (but on BLMOVE RIGHT LEFT we'll propagate an LMOVE) |
merged.. @felipou thanks a lot for sticking that long and implementing all the feedback. |
Hey, thank you all very much for all the feedback and for taking this in, especially you @oranagra! It was my first significant contribution to an important open-source project, and I have to say, the experience was great. Looking forward to contribute more! |
Adding [B]LMOVE <src> <dst> RIGHT|LEFT RIGHT|LEFT. deprecating [B]RPOPLPUSH. Note that when receiving a BRPOPLPUSH we'll still propagate an RPOPLPUSH, but on BLMOVE RIGHT LEFT we'll propagate an LMOVE improvement to existing tests - Replace "after 1000" with "wait_for_condition" when wait for clients to block/unblock. - Add a pre-existing element to target list on basic tests so that we can check if the new element was added to the correct side of the list. - check command stats on the replica to make sure the right command was replicated Co-authored-by: Oran Agra <oran@redislabs.com>
Adds lpoprpush, rpoprpush and lpoplpush, and the equivalent blocking commands: blpoprpush, brpoprpush and blpoplpush.