Skip to content
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

Add support for Redis 7's sharded pubsub commands #1066

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

slact
Copy link

@slact slact commented May 17, 2022

No description provided.

Copy link
Contributor

@bjosv bjosv left a comment

Choose a reason for hiding this comment

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

I believe we need to handle global channels (SUBSCRIBE) and sharded channels (SSUBSCRIBE) separately. It seems that the ac->sub.channels callback is used for both types, but according to the note in Redis docs they are separate.

@slact
Copy link
Author

slact commented May 18, 2022

As I understand, there is no difference between the MESSAGEs sent to subscribers from PUBLISH or SPUBLISH commands. Both send this to subscribers:

1) MESSAGE
2) <channel_name>
3) <message_data>

So if you use hiredis to SUBSCRIBE and also SSUBSCRIBE to channel "foo", when messages arrive it will be impossible to tell if it came from the sharded or the regular channel. While the pubsub and sharde-pubsub keyspaces are non-overlapping on the server-side, the protocol's failure to distinguish between the source of the message overlaps the keyspaces for the client.

By the way, I initially implemented this with a separate sub.sharded_channels dictionary for SSUBSCRIBE commands, but only later realized there's no way to actually route messages to the sub.channels or sub.sharded_channels dictionary on a given channel name.

@bjosv
Copy link
Contributor

bjosv commented May 23, 2022

As I understand, there is no difference between the MESSAGEs sent to subscribers from PUBLISH or SPUBLISH commands.

Ah, you are right! That's an interesting twist..

@bjosv
Copy link
Contributor

bjosv commented May 23, 2022

There are ongoing discussions regarding this problem in redis/redis#10748

@slact
Copy link
Author

slact commented May 24, 2022

Oh boy. Well, in the meantime, the official C library does not support the official API.

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.

2 participants