-
Notifications
You must be signed in to change notification settings - Fork 467
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
Add the support of the BLMPOP command #1774
Conversation
Hi @HolyLow , thank you for your contribution. Could you consider to rewrite it in our new helper class BlockingCommander? You can refer to CommandBPop for how to use it. |
@PragmaTwice seems we've done the same thing separately... Sure, I would refactor the code. As the design is very similar, it wouldn't take long. |
@PragmaTwice refactored to use |
@PragmaTwice A CI test case failed with Ubuntu 20.04, clang, TLS on. The test case is in the tls_test (no command logic involved?), and I try my best to try to reproduce the problem on my local environment (Ubuntu22.04, clang-16, TLS on), but the error could not be reproduced... I'm not aware what I should do now, could you please help? |
It's a flaky test case, can ignore this failure for now. We're still investigating. |
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.
LGTM
This closes #1522.
This PR moves all the blocking-event related code to a upper class
BlockedPopCommand
, as all the blocking commands would share these utils.If the
BlockedPopCommand
design gets accepted, I would try to refactor the other blocking commands, such asBLPOP
,BRPOP
andBLMOVE
to reduce the code redundancy, just as commented by @git-hulk in previous discussion.Any suggestions are welcomed.