Change sharded PubSub message type to smessage

[leibale]

With the current implementation of sharded PubSub there is no way to distinguish between "regular" and sharded PubSub messages:

> SUBSCRIBE channel
1) subscribe
2) channel
> SSUBSCRIBE channel
1) ssubscribe
2) channel

> PUBLISH channel message
(integer) 1

1) "message"
2) "channel"
3) "message"

> SPUBLISH channel message
(integer) 1

1) "message"
2) "channel"
3) "message"

Changing "message" to "smessage" will solve the issue, and it follows the same pattern as "message"

> SPUBLISH channel message
(integer) 1

1) "smessage"
2) "channel"
3) "message"

[itamarhaber]

Makes sense to me - @madolson what sayeth thee?

[hpatro]

@leibale Trying to understand the issue. Won't the client be able to figure out based on the command used to publish/subscribe?

[chayim]

CC @dvora-h you wanted to track this as well

[leibale]

@hpatro if one client subscribes to the same channel name using SUBSCRIBE and SSUBSCRIBE there is no way to tell if an incoming message was published using PUBLISH or SPUBLISH

[hpatro]

@leibale Thanks for the clarification.

I also feel we should make this breaking change. Once, the redis-core-team approves, will put up the PR.

[madolson]

@leibale You can always trivially differentiate them just with custom naming on the channels. However, I think for clients having the separate message allows them to more appropriately route them. (if message, forward to normal listener, if smessage forward to sharded listeners)

This seems like a more appropriate fix, and seems acceptable at this point.

[oranagra]

Other than the fact it's a breaking change for something that's already released (maybe it's early enough to afford to break this), maybe for some clients seeing them the same could be beneficial?

i.e. if the client library already handles message correctly, and the user's use the new commands via some execute_command mechanism without proper support from the client library, using the old format would maybe still allow you to use the existing mechanism, and if we change it, you won't be able to use any of that without real support from the client library?
I haven't looked into it at all, just speculating.

@yossigo please comment too.

[yossigo]

@oranagra I'm also not sure about the value here. @leibale Can you elaborate on the use case? I understand it's not possible to distinguish PUBLISH from SPUBLISH at the moment, but as @oranagra mentioned this can also be viewed as a good design choice.

[leibale]

@oranagra @yossigo

await client.SUBSCRIBE('channel', message => {
  console.log('SUBSCRIBE', message);
});

await client.SSUBSCRIBE('channel', message => {
  console.log('SSUBSCRIBE', message);
});

await client.PUBLISH('channe', 'message');

PUBLISH channel message

what the client should do? throw an error? run both handlers?

[zuiderkwast]

If it's not too late, I think we should change it now, so we don't have to live with this for the rest of our lives. We already have pmessage for PSUBSCRIBE, so tagging the message with smessage for SSUBSCRIBE is consistent.

If we don't fix it, clients will need to check both lists of handlers to know which callback(s) to call, which probably works in practice but it's extra complexity for the client.

I don't believe cluster clients supporting SUBSCRIBE can support SSUBSCRIBE out of the box. I can see how it can work if the client has a single callback regardless of channel, but that'd be a very simplistic client.

[mp911de]

What is the reason to distinguish between these two message types and why would someone want this distinction? I'm strongly in favor of keeping message as such a change can easily break clients.

[slact]

I am also opposed to making a separate SMESSAGE type. First off, the term itself makese little sense. Whereas SSUBSCRIBE and SPUBLISH have that S in the name to distinguish the sharded vs global pubsub namespace, MESSAGEs are generated by channels regardless of the subscription type.

By that I mean whether you SUBSCRIBEd or PSUBSCRIBEd to a channel, you still get a MESSAGE. Pattern-based subscriptions don't get you a PMESSAGE, so why should SSUBSCRIBEs get you SMESSAGEs?

Let's look at Redis < 7. Why do we keep the callbacks for SUBSCRIBE and PSUBSCRIBE in separate tables? They don't come from different namespaces. It's because the matching algorithms for what callback to run are different. So to keep things neat and efficient in the client libraries, we split up lookup-based matches and pattern-based matches.

But what if we have a client that SUBSCRIBEs and PSUBSCRIBEs to the same channel? Both callbacks would match the message coming from that channel, but which one to fire? This is a question left to answer by the client library's developers. Maybe both, maybe just one. You can extend this reasoning to multiple PSUBSCRIBEs that match the same channel is well.

So clients must already deal with the ambiguity of SUBSCRIBE and PSUBSCRIBE callbacks matching a channel's MESSAGE. Why, then, does SSUBSCRIBE warrant special treatment? It's trivial to implement by re-using the SUBSCRIBE table for SSUBSCRIBE commands -- the matching logic is just a string comparison after all.

TL;DR: PSUBSCRIBE and SUBSCRIBE already have ambiguous callback handling. it makes no sense to disambiguate for SSUBSCRIBE with an all-new breaking SMESSAGE.