feat(server): Add PUBSUB SHARDCHANNELS/SHARDNUMSUB #4702

mkaruza · 2025-03-05T08:14:30Z

Add support for PUB SHARDCHANNELS and PUB SHARDNUMSUB and report error back if sub command is not allow to run in non cluster mode.

resolves #847

adiholden · 2025-03-05T09:38:48Z

src/server/main_service.cc

@@ -2362,9 +2367,6 @@ void Service::Monitor(CmdArgList args, const CommandContext& cmd_cntx) {
 void Service::Pubsub(CmdArgList args, const CommandContext& cmd_cntx) {
  auto* rb = static_cast<RedisReplyBuilder*>(cmd_cntx.rb);

-  if (IsClusterEnabled()) {


shouldnt we reject all PUBSUB commands on cluster mode? @kostasrim

No not all, cause PUBSUB is a family (it has subcommands). So for example https://redis.io/docs/latest/commands/pubsub-shardchannels/#:~:text=An%20active%20shard%20channel%20is,glob%2Dstyle%20pattern%20are%20listed.

PUBSUB SHARDCHANNELS is fine, since shard channels are a notion of cluster.

However, PUBSUB CHANNELS should be rejected (until we have global pub sub available)

Ok, I understand. And the implementaion is just the same..
So if on cluster we allow all commands that subcommand start with "SHARD",
then I think it will be more readable to write the condition
if IsClusterEnabled() && !absl::StartsWith(subcmd, "SHARD") in this function
and remove the bool reject_cluster

Ok, I understand. And the implementaion is just the same..

Yes I did not decouple them because we had no design doc for global pub sub so I left it as is on purpose. I did not want to make decisions for the future

+1 for the rest

@adiholden thank you for suggestion, but this is problematic because it just checks SHARD prefix of subcommand and if that command is not valid it will still report error instead of returning warning about wrong subcommand (also, per @kostasrim review we need to cover situation when SHARD subcommands is used in non cluster environment).

@kostasrim I checked now the Redis documentation and it says
PUBSUB's replies in a cluster only report information from the node's Pub/Sub context, rather than the entire cluster.
This means we do support this commands in cluster mode, and the behavior is just like the shard sub command.
see f.e https://redis.io/docs/latest/commands/pubsub-numpat/

kostasrim

First PR! Congratz 👨‍🍳

Left some minor comments.

Also, plz write a short description in the PR and reference the issue that you are closing by this PR.

For reference see #4696 (comment)

kostasrim · 2025-03-05T09:38:29Z

tests/dragonfly/cluster_test.py

+
+    with pytest.raises(redis.exceptions.ResponseError) as moved_error:
+        await c_nodes[0].execute_command("PUBSUB CHANNELS")
+    assert str(moved_error.value) == f"PUBSUB CHANNELS is not supported in cluster mode yet"
+
+    with pytest.raises(redis.exceptions.ResponseError) as moved_error:
+        await c_nodes[0].execute_command("PUBSUB NUMSUB")
+    assert str(moved_error.value) == f"PUBSUB NUMSUB is not supported in cluster mode yet"
+
+    with pytest.raises(redis.exceptions.ResponseError) as moved_error:
+        await c_nodes[0].execute_command("PUBSUB NUMPAT")
+    assert str(moved_error.value) == f"PUBSUB NUMPAT is not supported in cluster mode yet"


These are somewhat redundant. The unit test your introduced above suffice

Oh I miss read this. Keep these, they are usefull

They check that CHANNELS, NUMBSUB, NUMPAT is not available in cluster.

Great!

kostasrim · 2025-03-05T09:39:56Z

tests/dragonfly/cluster_test.py

+        "data": 1,
+    }
+
+    message = await c_nodes[0].execute_command("PUBSUB SHARDCHANNELS")


nit: consider adding a few more channels, then check that they are printed here. Use a filter below, to check that we do actually filter and apply pattern correctly (which existed before and it should be correct but why not 😄 )

tests/dragonfly/cluster_test.py

kostasrim · 2025-03-05T09:50:50Z

src/server/cluster/cluster_family_test.cc

@@ -625,7 +625,12 @@ TEST_F(ClusterFamilyTest, ClusterModePubSubNotAllowed) {
              ErrArg("PSUBSCRIBE is not supported in cluster mode yet"));
  EXPECT_THAT(Run({"PUNSUBSCRIBE", "ch?"}),
              ErrArg("PUNSUBSCRIBE is not supported in cluster mode yet"));
-  EXPECT_THAT(Run({"PUBSUB", "CHANNELS"}), ErrArg("PUBSUB is not supported in cluster mode yet"));
+  EXPECT_THAT(Run({"PUBSUB", "CHANNELS"}),


Add the opposite checks for non cluster mode. That is PUBSUB SHARDCHANNELS gets rejected in non cluster mode

Ignore me, I missed that you included those. 💯

Using SHARDCHANNELS and SHARDNUMSUB in non cluster mode will work, i'll add this check and update tests

kostasrim · 2025-03-06T07:29:58Z

src/server/main_service.cc

    string_view pattern;
+    if ((subcmd == "CHANNELS" && IsClusterEnabled()) ||


small nit: I would store the comparison subcmd == "CHANNELS and subcmd == "SHARDCHANNELS into a variable. I would also centralize the checks above in one place:

if (IsClusterEnabled() && (IsChannels || IsNumSub())` and one for non cluster if (!IsClusterEnabled() && (IsShardChannels || blah blah))

That way we get checks at the top of the function and we avoid some of the extra checks.

adiholden · 2025-03-06T08:15:49Z

src/server/main_service.cc

-  if (subcmd == "CHANNELS") {
+  auto cmd_err = [&]() {
+    auto err = absl::StrCat("PUBSUB ", subcmd, " is not supported",
+                            IsClusterEnabled() ? " in cluster mode yet" : " in non cluster mode");


@kostasrim I think that the check should be IsClusterEnabledOrEmulated() right?
cause we should enable cluster commands in emulated mode

Oh we have Emulated() as well. Good catch. We also need to fix sharded pub sub as well

@mkaruza Dragonfly has emulated cluster mode which runs a single shard and in this state cluster and non cluster client are allowed.
This means that we should allow global pubsub command to run both on non cluster and emulated mode and sharded pubsub commands to run on both cluster and emulated mode, hence the check to send the error below should be fixed

adiholden · 2025-03-06T13:54:23Z

src/server/server_family_test.cc

@@ -547,4 +547,18 @@ TEST_F(ServerFamilyTest, CommandDocsOk) {
  EXPECT_THAT(Run({"command", "docs"}), "OK");
 }

+TEST_F(ServerFamilyTest, PubSubCommandErr) {
+  auto* flag = absl::FindCommandLineFlag("cluster_mode");


if (auto cluster_mode = absl::GetFlag(FLAGS_cluster_mode); cluster_mode == "")

adiholden · 2025-03-06T15:22:38Z

@mkaruza I can see that HllFamilyTest.Promote fail on the CI but its not related to your changes.
Please open a ticket with the link to this CI run so we will be able to follow up on this fail and after this you can rerun the action so you will be able to merge the PR

Signed-off-by: mkaruza <mario@dragonflydb.io>

We need to sort results because result is not deterministic

mkaruza requested a review from kostasrim March 5, 2025 08:14

mkaruza force-pushed the mkaruza/github#847 branch 2 times, most recently from 026e56a to 11173a4 Compare March 5, 2025 08:23

adiholden reviewed Mar 5, 2025

View reviewed changes

kostasrim reviewed Mar 5, 2025

View reviewed changes

mkaruza requested a review from kostasrim March 5, 2025 14:20

kostasrim previously approved these changes Mar 6, 2025

View reviewed changes

adiholden reviewed Mar 6, 2025

View reviewed changes

mkaruza dismissed kostasrim’s stale review via 7749f10 March 6, 2025 09:54

mkaruza force-pushed the mkaruza/github#847 branch from 864ff85 to 7749f10 Compare March 6, 2025 09:54

mkaruza requested a review from kostasrim March 6, 2025 10:00

adiholden reviewed Mar 6, 2025

View reviewed changes

adiholden approved these changes Mar 6, 2025

View reviewed changes

mkaruza added 8 commits March 6, 2025 16:55

feat(server): Add PUBSUB SHARDCHANNELS/SHARDNUMSUB

28f63a7

Signed-off-by: mkaruza <mario@dragonflydb.io>

Formated cluster_test.py

e83ce36

Fixed cluster_familiy_test

4a67eba

Changes according to PR review

a7057ba

Sort result list in cluster_test.py

928519f

We need to sort results because result is not deterministic

Move subcommand error reporting before if switch

8a9bfbf

Update check for PUBSUB subcommands

3193da3

USE flag for checking cluster_mode

Loading
Loading status checks…

8f622a4

mkaruza force-pushed the mkaruza/github#847 branch from e759608 to 8f622a4 Compare March 6, 2025 15:55

mkaruza merged commit 76f36cd into main Mar 6, 2025
10 checks passed

mkaruza deleted the mkaruza/github#847 branch March 6, 2025 17:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(server): Add PUBSUB SHARDCHANNELS/SHARDNUMSUB #4702

feat(server): Add PUBSUB SHARDCHANNELS/SHARDNUMSUB #4702

mkaruza commented Mar 5, 2025 •

edited

Loading

adiholden Mar 5, 2025

kostasrim Mar 5, 2025

kostasrim Mar 5, 2025

adiholden Mar 5, 2025

kostasrim Mar 5, 2025

mkaruza Mar 5, 2025

adiholden Mar 6, 2025

kostasrim left a comment

kostasrim Mar 5, 2025

kostasrim Mar 5, 2025

kostasrim Mar 5, 2025

kostasrim Mar 5, 2025

kostasrim Mar 5, 2025

kostasrim Mar 5, 2025

mkaruza Mar 5, 2025

kostasrim Mar 6, 2025

adiholden Mar 6, 2025

kostasrim Mar 6, 2025

adiholden Mar 6, 2025

adiholden Mar 6, 2025

adiholden commented Mar 6, 2025

		string_view pattern;
		if ((subcmd == "CHANNELS" && IsClusterEnabled()) \|\|

feat(server): Add PUBSUB SHARDCHANNELS/SHARDNUMSUB #4702

feat(server): Add PUBSUB SHARDCHANNELS/SHARDNUMSUB #4702

Conversation

mkaruza commented Mar 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostasrim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adiholden commented Mar 6, 2025

mkaruza commented Mar 5, 2025 •

edited

Loading