Fixed CROSSSLOT ERROR in Cluster Mode for PUBSUB SHARDNUMSUB

[Nikhil-Manglore]

Resolved #560

Added key specification to the pubsub-shardnumsub.json file to force the CROSSSLOT error when a user subscribes to two channels in different hash slots.

Prior to this change, running the PUBSUB SHARDNUMSUB command in cluster mode with multiple channels mapping to different slots (e.g., PUBSUB SHARDNUMSUB ch1 ch2) would succeed. It would also return 0 for the channel/slots it's not serving.

127.0.0.1:6379> PUBSUB SHARDNUMSUB ch1 ch2
1) "ch1"
2) (integer) 0
3) "ch2"
4) (integer) 0

With this PR, the command will now return a CROSSSLOT error if the provided channels map to different hash slots.

127.0.0.1:6379> PUBSUB SHARDNUMSUB ch1 ch2
(error) CROSSSLOT Keys in request don't hash to the same slot

[hpatro]

@Nikhil-Manglore Could you also add test case for it?

[zuiderkwast]

In the issue, we marked it as a breaking change, so should we release it in a major version, 9.0? Or should we consider it a bugfix and release it in 8.1? @valkey-io/core-team I guess we should decide.

[madolson]

It's undeniably a breaking change, since we are throwing errors when something used to work. It should go with 9.0.

[zuiderkwast]

@Nikhil-Manglore Since this is a breaking change, we will release it only in the next major version, which is 9.0. This means that we can't merge it until after we've branched off for the 8.1 release. We'll do that around January or February. Sorry for the inconvenience.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.81%. Comparing base (a401e37) to head (4f9f7f5).
⚠️ Report is 591 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #1369      +/-   ##
============================================
+ Coverage     70.69%   70.81%   +0.12%     
============================================
  Files           118      118              
  Lines         63549    63549              
============================================
+ Hits          44924    45005      +81     
+ Misses        18625    18544      -81     

Files with missing lines	Coverage Δ
src/commands.def	100.00% <ø> (ø)

... and 9 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Nikhil-Manglore]

When writing the test cases in tcl I realized there was an issue with the keyspecs in my original code. I fixed that issue by changing the starting position and then tested it with the testcases. This gave the proper results and i have pushed the final changes including the tcl testcases.

[hpatro]

@Nikhil-Manglore Thanks for the PR. As Viktor mentioned it's a breaking change, so we will merge this in for the major version (need to wait it out).

[hpatro]

@Nikhil-Manglore Would you be able to add before and after behavior to the top level comment?

@valkey-io/core-team This is a breaking change. So, I believe there should be a vote 👍 / 👎 .

[zuiderkwast]

@valkey-io/client-maintainers Do you think this will affect any clients?

[rueian]

@valkey-io/client-maintainers Do you think this will affect any clients?

Thanks for notifying. Valkey-go will need a small update to make its command builder to take those channel names as routing keys.

[Nikhil-Manglore]

I updated the top level description

[PingXie]

This change is incomplete. We'd also need to update SSUBSCRIBE if we would like to fail cross-slot requests.

However I don't think we should make this change without a off-by-default config, even in a major release.

Furthermore, I actually don't see a need to change this behavior. We don't need to subject shard-level channels to the global key namespace; rather, shard-level channels can be considered living in their own shard-level namespaces, which don't concern "slots" at all. It is just yet another implementation coincidence that we currently organize shard level channels using a CRC16 hash of their names. More specifically, I think it is fair to consider channel_1 in shard A being different than channel_1 in shard B in the context of sharded pubsub.

[ikolomi]

@valkey-io/client-maintainers Do you think this will affect any clients?

TL;DR: Either no effect or tests breakage

valkey-glide: no effect
valkey-py: Probable CI test breakage
libvalkey: no effect
valkey-go: The command builder is already ensures all the channels map to the same slots, but the CI test will break
valkey-java: CI test breakage
iovalkey: no effect

[zuiderkwast]

This change is incomplete. We'd also need to update SSUBSCRIBE if we would like to fail cross-slot requests.

@PingXie No, SSUBSCRIBE already has the key specs and would return -CROSSSLOT if attempting to subscribe to shard channels hashing to different slots.

Furthermore, I actually don't see a need to change this behavior. We don't need to subject shard-level channels to the global key namespace; rather, shard-level channels can be considered living in their own shard-level namespaces, which don't concern "slots" at all. It is just yet another implementation coincidence that we currently organize shard level channels using a CRC16 hash of their names. More specifically, I think it is fair to consider channel_1 in shard A being different than channel_1 in shard B in the context of sharded pubsub.

This is not how sharded pubsub works though. Shard channels are hashed to slots just like keys. This is implemented in most of the sharded pubsub commands, including SSUBSCRIBE, SPUBLISH, etc. Only PUBSUB SHARDNUMSUB seems to be missing.

[madolson]

Vote for breaking the change. 👍 is for breaking change and 👎 is for updating the documentation.

[hpatro]

Vote for breaking the change. 👍 is for breaking change and 👎 is for updating the documentation.

The vote is pretty confusing. Please vote for one @valkey-io/core-team 😜.

[avifenesh]

Vote for breaking the change. 👍 is for breaking change and 👎 is for updating the documentation.

This is a significant breaking change for numerous existing tools that rely on Valkey (or that are currently using Redis and wish to incorporate support for Valkey) because the tools are predicated on specific behaviors.
After the Valkey release, all tools and libraries will need to have a major version release to change tool behavior.
It won't affect the clients' support, but many big tools that use PubSub will stop working for no reason unless the clients cover this behavior by not responding to it at all.
Numerous popular libraries use PubSub as their main logic; for many, it is more frequently used than get/set, so breaking it should be carefully considered.
From a client integration perspective, I don't think Valkey is ready to break so many existing tools that already support other in-memory key-value solutions, and not enough of the tools make Valkey the default option. The attempt to incorporate Valkey into those tools, even if only semantically, will be hampered.
Please think carefully about this.

[hpatro]

@avifenesh thanks for sharing your thoughts. This is limited to sharded pubsub which I believe is still maturing and could be improved. Happy to hear client devs feedback though.

Could you also share how GLIDE deals with the above issue currently?

[avifenesh]

@avifenesh thanks for sharing your thoughts. This is limited to sharded pubsub which I believe is still maturing and could be improved. Happy to hear client devs feedback though.

Could you also share how GLIDE deals with the above issue currently?

#1369 (comment) for glide, it won't affect the behavior as @ikolomi mentioned. For a detailed answer, he can add more information.
The last time something similar (or the same, I'm not sure) was introduced and then reverted, it had a large impact, and many libraries had discussions in reaction to that.

[zuiderkwast]

Every two months or so, I go over all PRs marked with major-decision-pending and read up on the details that have been forgotten since last time, which takes time, only to realize that we're never going to approve, nor reject, this one (among others).

Apparently, fixing this is a breaking change and it's better to leave it broken.

I'm going to close this one now. If anyone thinks it's worth discussing again, it's possible to reopen it.