Skip to content

feat: add support for BLMPOP command #5370

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

Merged
merged 3 commits into from
Jul 1, 2025
Merged

Conversation

EricHayter
Copy link
Contributor

@EricHayter EricHayter commented Jun 26, 2025

Adding support for the BLMPOP redis command. The command description can be found here.

Resolves #3876

@EricHayter
Copy link
Contributor Author

EricHayter commented Jun 26, 2025

I'm working on implementing the test cases but I ran into a problem when providing non-integer or a 0 to the timeout argument. It seems like the timeout argument is being parsed as a the numkeys argument.

127.0.0.1:6379> BLMPOP 0 1 empty empty LEFT COUNT 10
(error) ERR at least 1 input key is needed for this command
127.0.0.1:6379> BLMPOP 0.01 1 empty empty LEFT COUNT 10
(error) ERR value is not an integer or out of range

It could be an issue with how the command is being registered but it's not clear what values should be used instead of the following?:

<< CI{"BLMPOP", CO::WRITE | CO::SLOW | CO::VARIADIC_KEYS, -5, 3, 3, acl::kBLMPop}.HFUNC(
             BLMPop)

I did check the redis arguments (as suggested in command_id.h) but it's not clear what I would use as the last key index?

@romange
Copy link
Collaborator

romange commented Jun 26, 2025

yeah, sorry for this. you probably need to hack DetermineKeys function in transaction.cc.
I am guessing num_keys_index is deduced incorrectly. The command structure in redis is not uniform and we wrote a single, not very elegant function that handles "exceptions".

@EricHayter EricHayter force-pushed the main branch 2 times, most recently from 0613547 to 5d4e7ea Compare June 27, 2025 18:03
This commit adds support for the `BLMPOP` command, which is the blocking
variant of the existing `LMPOP` command.

This commit resolves GitHub issue dragonflydb#3876

Signed-off-by: Eric <hayter.eric@gmail.com>
@EricHayter EricHayter marked this pull request as ready for review June 27, 2025 18:06
@EricHayter EricHayter changed the title feat: Add support for BLMPOP command feat: add support for BLMPOP command Jun 27, 2025
Copy link
Collaborator

@romange romange left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good! a few minor comments

@EricHayter EricHayter requested a review from romange June 29, 2025 01:11
@romange
Copy link
Collaborator

romange commented Jul 1, 2025

Thank you @EricHayter

@romange romange closed this Jul 1, 2025
@romange romange reopened this Jul 1, 2025
@romange romange merged commit 2598f97 into dragonflydb:main Jul 1, 2025
10 checks passed
@romange
Copy link
Collaborator

romange commented Jul 1, 2025

@EricHayter there is one thing that is missing - CO::NO_AUTOJOURNAL. Basically for such commands we need to send a different command for replication, so we disable the default journaling. Also OpPop should be called with journal_rewrite=true

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

implement BLMPOP
2 participants