Feat: Added BZMPOP Command #5436

H4R5H1T-007 · 2025-07-09T02:30:07Z

romange · 2025-07-09T06:00:35Z

src/server/zset_family.cc

@@ -1819,12 +1819,23 @@ std::optional<std::string_view> GetFirstNonEmptyKeyFound(EngineShard* shard, Tra
  return std::nullopt;
 }

-// Validates the ZMPop command arguments and extracts the values to the output params.
+// Validates the ZMPop and BZMPop command arguments and extracts the values to the output params.
 // If the arguments are invalid sends the appropiate error to builder and returns false.
 bool ValidateZMPopCommand(CmdArgList args, uint32* num_keys, bool* is_max, int* pop_count,


Ok this function has not a very nice interface. Please introduce ValidateZMPopResult consisting of num_keys, is_max, pop_count and timeout.
The interface should be something like this: bool (CmdArgList args, bool is_blocking, SinkReplyBuilder* builder, ValidateZMPopResult* result)

romange · 2025-07-09T17:29:11Z

src/server/zset_family.cc

+    auto* ns = &trans->GetNamespace();
+
+    // Concluding transaction if it's multi mode as blocking is not allowed in it.
+    if (trans->IsMulti()) {


I would unify this and the block at line 2509 before you handle the blocking case:

if (!key_to_pop && (!is_blocking || trans->IsMulti())) { ....

romange · 2025-07-09T17:31:32Z

src/server/zset_family.cc

@@ -2783,6 +2850,9 @@ void ZSetFamily::Register(CommandRegistry* registry) {
             ZInterCard)
      << CI{"ZLEXCOUNT", CO::READONLY, 4, 1, 1, acl::kZLexCount}.HFUNC(ZLexCount)
      << CI{"ZMPOP", CO::SLOW | CO::WRITE | CO::VARIADIC_KEYS, -4, 2, 2, acl::kZMPop}.HFUNC(ZMPop)
+      << CI{"BZMPOP",    CO::SLOW | CO::WRITE | CO::VARIADIC_KEYS | CO::BLOCKING, -5, 3, 3,


you also need CO::NO_AUTOJOURNAL to replicate this correctly. this also requires a emitting a custom journal command. Look at the other NO_AUTOJOURNAL commands.

romange · 2025-07-14T05:23:07Z

src/server/zset_family.cc

+      // if command conatins no_autojournal and it's BZMPOP then we will write to journal manually.
+      if ((result == OpStatus::OK) && (cid->opt_mask() & CO::NO_AUTOJOURNAL) &&
+          (cid->name() == "BZMPOP")) {
+        OpArgs op_args = t->GetOpArgs(shard);


I just checked and actually we have the same bug in ZMPOP. Even with ZMPOP, there is no need to pass all the keys because the outcome is already known: we need to fetch from key.
Therefore, I suggest to move RecordJournal into OpPopCount and rewrite both commands as ZPOPMIN or ZPOPMAX accordingly. Also please do not forget to mark ZMPOP as NO_AUTOJOURNAL.

Look at how it's done in OpBZPop. Thanks!

Yup I also thought that but then I thought to make it consistent as ZMPOP so I added it like ZMPOP journal. Cool I will move journaling to OpPopCount and add custom journal to ZMPOP as well that will be much better.

romange

Almost there. I appreciate your effort!

romange · 2025-07-14T05:24:40Z

src/server/zset_family.cc

-bool ValidateZMPopCommand(CmdArgList args, uint32* num_keys, bool* is_max, int* pop_count,
-                          SinkReplyBuilder* builder) {
+bool ValidateZMPopCommand(CmdArgList args, ZSetFamily::ValidateZMPopResult* result,
+                          SinkReplyBuilder* builder, bool is_blocking) {


nit: we put output arguments at the end, so is_blocking should be second here

romange · 2025-07-14T05:26:00Z

src/server/zset_family.h

@@ -66,6 +67,13 @@ class ZSetFamily {
    bool override = false;
  };

+  struct ValidateZMPopResult {


you do not need this struct to be defined in the header file. please move it to the cc file.

romange · 2025-07-14T05:27:20Z

src/server/zset_family.h

@@ -66,6 +67,13 @@ class ZSetFamily {
    bool override = false;
  };

+  struct ValidateZMPopResult {
+    uint32* num_keys;


no need to pass pointers. you can just define:

struct ValidateZMPopResult { uint32_t num_keys; bool is_max; int pop_count; float timeout; };

and everything will be much simpler in the code.

romange · 2025-07-14T05:27:34Z

src/server/zset_family.h

@@ -7,6 +7,7 @@
 #include <string_view>
 #include <variant>

+#include "base/integral_types.h"


romange · 2025-07-14T05:28:43Z

tests/dragonfly/replication_test.py

+        # check BZMPOP turns into ZMPOP command
+        await c_master.zadd("key", {"a": 1, "b": 2, "c": 3})
+        await skip_cmd()
+        await check("BZMPOP 0 3 key3 key2 key MAX COUNT 3", r"ZMPOP 1 key MAX COUNT 3")


very nice, thank you! but it should turn as ZPOPMAX/ZPOPMIN :)

Sure I will change it

romange · 2025-07-16T15:15:42Z

src/server/zset_family.cc

-  if (!ValidateZMPopCommand(args, &num_keys, &is_max, &pop_count, cmd_cntx.rb)) {
+// Generic function for ZMPop and BZMPop commands
+void ZMPopGeneric(CmdArgList args, const CommandContext& cmd_cntx, bool is_blocking) {
+  ValidateZMPopResult zmpopArgs;


naming: zmpop_args

romange

minor comments

romange · 2025-07-16T15:16:21Z

src/server/zset_family.cc

  // Check if we have additional COUNT argument.
  if (parser.HasNext()) {
-    if (!parser.Check("COUNT", pop_count)) {
+    if (!parser.Check("COUNT", &(result->pop_count))) {


Suggested change

if (!parser.Check("COUNT", &(result->pop_count))) {

if (!parser.Check("COUNT", &result->pop_count)) {

romange · 2025-07-16T15:17:44Z

src/server/zset_family.cc

  CmdArgParser parser{args};

-  if (!SimpleAtoi(parser.Next(), num_keys)) {
+  if (is_blocking) {
+    if (!absl::SimpleAtof(parser.Next(), &(result->timeout))) {


Suggested change

if (!absl::SimpleAtof(parser.Next(), &(result->timeout))) {

if (!absl::SimpleAtof(parser.Next(), &result->timeout)) {

Also added test for BZMPOP custom journal by checking replication.

- Added Custom journaling for BZMPOP command. - Created ValidateZMPopResults Interface.

Added manual journaling for ZMPOP and BZMPOP which turns them into ZPOPMAX/ZPOPMIN. Also refactored the code a bit.

kostasrim mentioned this pull request Jul 9, 2025

Update BZMPOP to supported dragonflydb/documentation#386

Open

romange requested changes Jul 9, 2025

View reviewed changes

H4R5H1T-007 force-pushed the main branch from d95195b to c002a98 Compare July 14, 2025 05:12

romange reviewed Jul 14, 2025

View reviewed changes

H4R5H1T-007 force-pushed the main branch from c002a98 to ef453a7 Compare July 16, 2025 10:14

H4R5H1T-007 requested a review from romange July 16, 2025 11:01

romange reviewed Jul 16, 2025

View reviewed changes

H4R5H1T-007 added 6 commits July 17, 2025 10:02

Feat: Added BZMPOP Command functionality.

abf0ef8

Added tests for BZMPOP command.

55b84cb

Added Custom journal to BZMPOP command.

efd171e

Also added test for BZMPOP custom journal by checking replication.

Updated the PR with changes required.

1d8e667

- Added Custom journaling for BZMPOP command. - Created ValidateZMPopResults Interface.

Added Manual Journaling in OpPopCount.

72b6925

Added manual journaling for ZMPOP and BZMPOP which turns them into ZPOPMAX/ZPOPMIN. Also refactored the code a bit.

Added minor suggested changes.

344e925

H4R5H1T-007 force-pushed the main branch from ef453a7 to 344e925 Compare July 17, 2025 04:43

H4R5H1T-007 requested a review from romange July 17, 2025 04:48

	if (!parser.Check("COUNT", &(result->pop_count))) {
	if (!parser.Check("COUNT", &result->pop_count)) {

	if (!absl::SimpleAtof(parser.Next(), &(result->timeout))) {
	if (!absl::SimpleAtof(parser.Next(), &result->timeout)) {

Feat: Added BZMPOP Command #5436

Are you sure you want to change the base?

Feat: Added BZMPOP Command #5436

Conversation

H4R5H1T-007 commented Jul 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romange left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romange left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!