Sharded pubsub implementation #8621

hpatro · 2021-03-08T23:18:11Z

This is a PR to support a sharded pubsub implementation based on cluster hashing.

API Definitions can be found here

New commands introduced:

SPUBLISH
SSUBSCRIBE
SUNSUBSCRIBE

Prefix S denotes SHARD.

New Sub commands

PUBSUB SHARDCHANNELS
PUBSUB SHARDNUMSUB

New Configs

cluster-allow-pubsubshard-when-down: Controls whether local pubsub is allowed when the cluster is down (when a majority of slots can't be served or we can't reach a majority of primaries.) Unlike key based commands, which defaults to rejecting commands, pubusub by default allows pubsub messages, since they are best effort.

Considerations made:

Slot Migration and cleanup are handled. Slot migration works by having the migrating node continue to serve the pubsub channels, and at the very end of the migration clients will be redirected to the importing node.
Local channels are allocated to the same node as keys (same hashing logic).
Client(s) will be redirected to the master node of the slot if they try to connect to a node not serving the slot.
Client(s) can connect to replica node(s) serving the slot and perform operations.
READ_ONLY flag doesn't impact pubsublocal feature. As pubsub operations are neither read/write.
Global channels and local channels share the ACL rule(s).
Client(s) are sent a "unsubscribe " for each channel in the slot when on slot migration after applying SETSLOT <slot> NODE <node-id>. Clients will need to handle this message, and choose to either reconnect to the new target OR throw an exception up to the client. (Alternative to this is to disconnect the client)
A single subscribe can only connect to channels within a single slot. This simplifies various pieces of internal logic. Client(s) can connect to channels across multi slot over separate SUBSCRIBELOCAL calls though.
Client(s) can connect to both local and global channels together once in the subscribe state.
Client(s) would enter pubsub mode either on local/global channels subscription.
During the pubsub mode, only certain commands are allowed i.e. pingCommand, subscribeCommand, subscribeLocalCommand, unsubscribeCommand, unsubscribeLocalCommand, psubscribeCommand, punsubscribeCommand and resetCommand.

Additional considerations:

PSUBSCRIBELOCAL (Pattern subscribe) is currently not implemented, but has no technical blockers. The motivation is that the behavior is less obvious which pattern should be associated with which slot. The decision for now what to omit it for now.

madolson

I didn't take a super deep look as this is still a draft, but looks consistent with what we talked about.

@redis/core-team Might be useful to take a look now while this is in the early draft stage for how this solves the cluster pubsub scaling problem. The interesting bit is that it's introduce 2 new commands publishlocal and subscribelocal, which exist in their own namespaces.

Full suggestion is here: #8029

src/pubsub.c

src/server.c

src/pubsub.c

madolson · 2021-03-09T18:04:05Z

src/pubsub.c

Since this is "local", should we be propagating it? Some thoughts here:

Always require it on a primary and then replicate it normally.

Don't replicate it and require consistent behavior from the clients about where you are connecting.

I believe we should replicate it within the (master, replica) set. Otherwise, I believe we are restricting the data to a single instance.

I don't see any advantage to always publish the data on master. We should allow the client to publish data irrespective of a master or a replica.

In cluster mode, you can issue publish commands to replicas and they get propagated to primaries through the clusterbus. Pubsub local doesn't do that, it expects it to go to the primary to be replicated. This seems like weird behavior to break. This is especially odd since we don't make publish consistent (at least I think) and don't redirect it to primaries by default.

Thanks for clarifying this. I've fixed the implementation to support pubsub local commands to work on the master/replica and data broadcast to happen through clusterbus within a slot.

src/pubsub.c

madolson

Solid progress, I did look at everything except the tests.

src/server.c

madolson · 2021-06-01T04:08:26Z

src/pubsub.c

In cluster mode, you can issue publish commands to replicas and they get propagated to primaries through the clusterbus. Pubsub local doesn't do that, it expects it to go to the primary to be replicated. This seems like weird behavior to break. This is especially odd since we don't make publish consistent (at least I think) and don't redirect it to primaries by default.

src/server.h

src/redis-cli.c

src/pubsub.c

src/cluster.c

src/pubsub.c

madolson · 2021-07-19T22:17:24Z

@redis/core-team Hey, wanted to get your guy's opinions about a couple of open questions. Skip to the third section if you are aware of the context. @zuiderkwast I know you're away, but take a look when you're back as well.

High level changes

Three new commands will be introduced: publishlocal, subscribelocal, unsubsribelocal that add support for "local channels". (Feel free to suggest alternative naming now)
Subscribing and publishing to a local channel works just like a regular channel, except it exists in a separate namespace and is bound to a slot. Example:
- Client A does a Localsubscribe to local channel foo
- Client B does a normal subscribe to channel foo
- Client C does a local publish to channel foo. Client A receives the message.
Local channels will be assigned to slots based on the CRC16 mod 2^14, like they are for regular keys.
Sending a local message to a server not serving the slot will be redirected to the primary that owns the slot.
Subscribing to a local channel on a server not serving the slot will be redirect to the primary that owns the slot.
Messages are broadcasted between nodes over the clusterbus and not via replication. This reduces write amplification from the number of nodes in the cluster to the number of nodes in the shard.
local channels are supported in cluster mode disabled, but don't provide any specific value.
Patterns are not in the initial scope.

Assumptions that are staying the same for local channels

Local messages only provide at most once delivery guarantees
There is no ordering guarantees between servers, if you are subscribing on a replica you might get a different order subscribing on the primary.
There is also no delivery guarantees for local messages originating from the same client. If you are client that sends 3 messages, the middle one can be dropped.(In this case the clusterbus can drop the message) Messages will not be re-ordered though.

Open questions

Here are the open questions that have come up while reviewing that the requester wanted to close on before continuing. They're summarized as multiple options, I've included my recommendation.

Should publishlocal be restricted just to primaries?
- Today only primaries own slots, replicas have a lagged view and might disagree. Therefor in a cluster down scenario a replica could accept a publish it doesn't know it should redirect (This might change with some of the Cluster V2). Forcing publishing to happen on primaries will give us a stronger consistency model, but it's pubsub, so it's not that important. <- My preference
- The reason against publishing just to primaries is that today cluster pubsub allows you to send to wherever you feel like. Since it's not generating any write traffic into the keyspace, publishing on replicas will better diffuse the load across the shard.
Should the channel be considered a "key" and be included in the key positions of "command". Also, internal implementation detail, should it be considered a "key" to simplify command routing?
- A channel is not a key, and should not be considered one. The Command command will list it as having no keys. Internally we will add code to getNodeFromQuery() for routing. <- My preference
- It's "basically a key", so might as well re-use the existing key specification (or used/extend the keyspec that was proposed). We will need to explicitly handle the cases where getKeys() is called like client side tracking.
What is the behavior of pubsublocal commands during slot migration? (Including replica migration) Only one option listed here, but maybe there are others?
- During migration, it is assumed that ALL channels are "immediately migrated" to the target. As soon as a slot is moved into the migrating state, all clients connected are disconnected.
- The migration process needs to start with the target being put into importing before the source is put into migrating. I believe that is the required path, but worth validating.
What is the "no longer serving slot behavior". I.E. when a replica migrates away or when a slot migration happen, what happens to clients subscribing to the local channel.
- Disconnect the client after all the pending messages have been written out. Clients should know how to handle this case, and seems consistent with other mechanisms we have today like ACL changes. <- My preferred option
- We can send a message indicating that the server is no longer serving a given slot. This requires clients to understand the message and take action. Seems more complex.
Can a single client subscribe to multiple different slots?
- Be consistent with cluster, and only allow consuming from a single slot at a given time. You can mix the global channels + local channels. <- My preferred option
- Allow consuming from multiple slots. This doesn't really break any consistency guarantees and allows for fewer connections. However it seems semantically wrong, since regular cluster forces you not to mix slots.
Should subscribing on a replica require the READONLY command to be executed:
- Yes, since replicas world view can be stale, it seems like we should maintain the assumption. ←My preference
- The counter argument is that pubsub has poor guarantees today, so it would unnecessary to require the READONLY command.

oranagra · 2021-07-20T19:29:15Z

not sure how i agree about these limitations:

local channels are not supported in cluster mode disabled.

Patterns are not in the initial scope.

regarding open questions:

i also feel that we want to only accept local publish in masters, seems that following the existing practice we won't run into new surprises.
i agree these are not keys, but we'll need COMMAND command to give some hints to clients one way or another (unless we expect them to handle this command explicitly). maybe with the changes we plan for this mechanism in 7.0 we can show them as keys, but also add some flag indicating that they're not. (some way of still supporting old clients with a good default).
i agree that the easy thing is to drop the client when the slot is migrated, it's also easier for the client maybe. but maybe we can add a feature in which the client declares ahead of time if it can handle a slot invalidation message, in which case we send one rather than drop it (more efficient in case multiple channels are used by one client), we can add such a feature in the future (not a must for the first version)
i think a single client should be able to subscribe to multiple slots. i also said at the top that i think this feature should be supported in non-cluster mode, i think these two go together.

i feel i'm not knowledgeable enough in these topics, so take my opinion with a grain of salt.

madolson · 2021-07-20T21:31:00Z

@oranagra

I updated to cluster mode being in scope, I suppose the initial thought was that there was no value, but it's also trivial to support, so there is no harm. The name doesn't make as much sense though?
Nothing explicitly requires patterns, if we feel strongly about that we can do it.
The cross slot subscribe is the position I hold the least strongly. One thought I've had in the past is that you should be able to send a command like CLIENT ALLOW-MULTI-SLOT, which is just a signal that you are making an optimization to fetch data from multiple slots. In this case, if you want to subscribe to multiple local channels, you just send that command first (similar to READONLY)

…s during migration.

YeonCheolGit · 2022-01-07T14:56:21Z

src/cluster.c

+            if (type == CLUSTERMSG_TYPE_PUBLISHSHARD) {
+                pubsubPublishMessageShard(channel, message);
+            } else {
+                pubsubPublishMessage(channel,message);


pubsubPublishMessageShard(channel, message) parameters have space but pubsubPublishMessage(channel,message); don't have space.

what about this? pubsubPublishMessage(channel, message);

oranagra · 2022-01-09T09:39:09Z

can someone look into this CI failure:
https://github.com/redis/redis/runs/4745050797?check_suite_focus=true

00:50:22> Disconnect link when send buffer limit reached: FAILED: Expected [get_info_field [::redis::redisHandle1666 cluster info] total_cluster_links_buffer_limit_exceeded] eq 1 (context: type eval line 36 cmd {assert {[get_info_field [$primary1 cluster info] total_cluster_links_buffer_limit_exceeded] eq 1}} proc ::test)

madolson · 2022-01-10T00:54:44Z

@oranagra Seems like that isn't related to this issue, seems related to the #9774 (comment)

oranagra · 2022-01-16T07:10:46Z

looks like the test hung here (saw it only once, so may be a false report)
https://github.com/redis/redis/runs/4829401183?check_suite_focus=true#step:9:632

madolson · 2022-01-17T02:08:42Z

@hpatro FYI. I'm not aware of any place it's likely to get stuck, let's keep a look out if it happens again.

tezc · 2022-01-17T11:13:26Z

Test hung on my fork : https://github.com/tezc/redis/runs/4834585930?check_suite_focus=true#step:9:630
I had some changes on this branch but don't think it effects this test.

hpatro · 2022-01-17T11:32:17Z

Taking a look.

tezc · 2022-01-17T12:56:25Z

00:46:57> Disconnect link when send buffer limit reached: FAILED: Expected [get_info_field [::redis::redisHandle1876 cluster info] total_cluster_links_buffer_limit_exceeded] eq 1 (context: type eval line 36 cmd {assert {[get_info_field [$primary1 cluster info] total_cluster_links_buffer_limit_exceeded] eq 1}} proc ::test)
(Jumping to next unit after error)

@oranagra @hpatro Just realized in both failures, previous test case failed. Maybe, this is not related to this PR at all. Maybe it's about #9774 , something is leaking to pubsub test after the failure. node-timeout is set to 1 hour in that test, maybe that's why pubsub test hangs? Just fyi.

elena-kolevska · 2022-01-25T21:39:10Z

Thanks for the work on this @hpatro! Sorry for commenting on an already closed PR but I just want to point out a small inconsistency between the behaviour of redis-cli for SUBSCRIBE and SSUBSCRIBE. Namely, when we run SSUBSCRIBE we don't enter pub sub mode as we do with SUBSCRIBE.
Let me know if you'd prefer this in an issue.

hpatro · 2022-01-26T08:29:40Z

@elena-kolevska I get the below view after SSUBSCRIBE, based on this change https://github.com/redis/redis/blob/unstable/src/redis-cli.c#L1418. Is there anything else expected ?

harkrisp@147dda4813f2 redis % src/redis-cli
127.0.0.1:6379> ssubscribe hp
Reading messages... (press Ctrl-C to quit)
1) "ssubscribe"
2) "hp"
3) (integer) 1

elena-kolevska · 2022-01-26T10:29:47Z

@hpatro I'm sorry, this was a silly (and embarrassing) mistake, I had ran redis-cli from my path by mistake, not from unstable. Works as expected from unstable.

hpatro · 2022-01-26T12:57:14Z

@elena-kolevska Ah, makes sense. Np.

publishshard was added in redis#8621 (7.0 RC1), but the publishshard_sent stat is not shown in CLUSTER INFO command. Other changes: 1. Remove useless `needhelp` statements, it was removed in 3dad819. 2. Use `LL_WARNING` log level for some error logs (I/O error, Connection failed). 3. Fix typos that saw by the way.

publishshard was added in #8621 (7.0 RC1), but the publishshard_sent stat is not shown in CLUSTER INFO command. Other changes: 1. Remove useless `needhelp` statements, it was removed in 3dad819. 2. Use `LL_WARNING` log level for some error logs (I/O error, Connection failed). 3. Fix typos that saw by the way.

We already have `pubsub_channels` and `pubsub_patterns` in INFO stats, now add `pubsubshard_channels` (symmetry). Sharded pubsub was added in redis#8621

We already have `pubsub_channels` and `pubsub_patterns` in INFO stats, now add `pubsubshard_channels` (symmetry). Sharded pubsub was added in #8621

We already have `pubsub_channels` and `pubsub_patterns` in INFO stats, now add `pubsubshard_channels` (symmetry). Sharded pubsub was added in redis#8621

madolson reviewed Mar 9, 2021

View reviewed changes

This was linked to issues Mar 9, 2021

[FEATURE] Keyspace based pubsub #8029

Closed

Redis Cluster PubSub - Scalability Issues #2672

Closed

madolson mentioned this pull request May 15, 2021

Redis Cluster V2 project #8948

Open

hpatro added 10 commits May 31, 2021 16:21

Initial commit of pubsub local command.

d3ceed2

pubsub local stats command.

30c5574

Unsubscribe local command.

cf1d7f2

Add/Remove to channel slot on pub/sub.

1cdfcd3

Refactoring and cleanup of slots on no subscription to channel.

29606af

Unsubscribe client on slot migration.

4aef1bc

Some rebase changes.

7c9f926

Base test cases for pubsublocal.

44b7c38

Fix client tracking/acl check and add cluster tests.

033a564

Add acl test for pub/sub local.

49057fa

hpatro force-pushed the pubsublocal branch from 0022ced to 49057fa Compare May 31, 2021 14:24

madolson suggested changes Jun 2, 2021

View reviewed changes

Add support for message propagation for a slot through clusterbus.

f47dae4

youurayy mentioned this pull request Jun 26, 2021

[META] Streams #5386

Open

hpatro added 2 commits July 3, 2021 18:04

Tests for message propagation for a slot through clusterbus.

cd9a941

Unsubscribe client on replica after slot migration.

26dfe7e

hpatro added 7 commits August 6, 2021 11:04

Uncomment test assertion.

6b5d3f1

Treat pubsublocal commands arguments as non keys.

edae943

Touch unused variable.

e4f67f9

Fix tests for validating cross slot subscription in a node.

c30d3e7

Fix build failures.

df3c3e9

Add tests to verify behaviour of client with cross slot channel acces…

aeb86d0

…s during migration.

Code cleanup and documentation.

0a09020

YeonCheolGit reviewed Jan 7, 2022

View reviewed changes

enjoy-binbin mentioned this pull request Feb 19, 2022

Show publishshard_sent stat in cluster info #10314

Merged

slorello89 mentioned this pull request Mar 8, 2022

Cannot get correct number of subscribers while using Redis clustering StackExchange/StackExchange.Redis#1328

Closed

sazzad16 mentioned this pull request Mar 16, 2022

Support Sharded PubSub redis/jedis#2948

Closed

2 tasks

madolson mentioned this pull request Apr 10, 2022

New approach for pubsub on cluster with no broadcast #5336

Closed

hpatro mentioned this pull request Apr 14, 2022

suggest add a option for redis-cluster(pub/sub) ,just broadcast message in the same sharding #3181

Closed

rueian mentioned this pull request Jun 26, 2022

Panic during reconnect to mulfunctional Redis Cluster redis/rueidis#55

Closed

This was referenced Jul 4, 2022

Add pubsubshard_channels field in INFO STATS #10929

Merged

The releasenotes incorrectly marked pubsubshard as pubsublocal. #10939

Closed

oranagra pushed a commit that referenced this pull request Jul 6, 2022

Add pubsubshard_channels field in INFO STATS (#10929)

0132ed7

We already have `pubsub_channels` and `pubsub_patterns` in INFO stats, now add `pubsubshard_channels` (symmetry). Sharded pubsub was added in #8621

cenzovit mentioned this pull request Aug 18, 2022

how can we use the benefits of redis streams with this socket.io-redis-adapter. socketio/socket.io-redis-adapter#420

Closed

enjoy-binbin mentioned this pull request Jun 20, 2023

Is there any possibility to add ability to SUBSCRIBE to pattern shared channels in future ? #12332

Open

madolson mentioned this pull request Mar 18, 2024

[Cluster] add cluster-pubsub-propagate config (default to yes) #3381

Closed

Sharded pubsub implementation #8621

Sharded pubsub implementation #8621

Uh oh!

Conversation

hpatro commented Mar 8, 2021 • edited by madolson Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

madolson left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

madolson Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

hpatro May 31, 2021

Choose a reason for hiding this comment

Uh oh!

madolson Jun 1, 2021

Choose a reason for hiding this comment

Uh oh!

hpatro Jun 21, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

madolson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

madolson Jun 1, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

madolson commented Jul 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

High level changes

Assumptions that are staying the same for local channels

Open questions

Uh oh!

oranagra commented Jul 20, 2021

Uh oh!

madolson commented Jul 20, 2021

Uh oh!

YeonCheolGit Jan 7, 2022

Choose a reason for hiding this comment

Uh oh!

oranagra commented Jan 9, 2022

Uh oh!

madolson commented Jan 10, 2022

Uh oh!

oranagra commented Jan 16, 2022

Uh oh!

madolson commented Jan 17, 2022

Uh oh!

tezc commented Jan 17, 2022

Uh oh!

hpatro commented Jan 17, 2022

Uh oh!

tezc commented Jan 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elena-kolevska commented Jan 25, 2022

Uh oh!

hpatro commented Jan 26, 2022

Uh oh!

elena-kolevska commented Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hpatro commented Jan 26, 2022

hpatro commented Mar 8, 2021 •

edited by madolson

Loading

madolson left a comment •

edited

Loading

madolson commented Jul 19, 2021 •

edited

Loading

tezc commented Jan 17, 2022 •

edited

Loading

elena-kolevska commented Jan 26, 2022 •

edited

Loading