Complete sharded Pub/Sub support - #74
Conversation
Add SPUBLISH to the shared command API and expose SSUBSCRIBE through the pooled client. Track ordinary, pattern, and sharded subscriptions separately so Subscription#close sends the correct unsubscribe command and consumes all pending acknowledgements Remove cluster-level subscription methods that bypass command routing or operate on an unrelated pooled connection, and replace timing-based Pub/Sub specs with deterministic integration coverage
|
Hi @jgaskins! Been a fan of your code. I was looking to complete sharded subscriptions last week so I could use it for a project, and then it just so happened that you added partial support for it a couple days later. I've gone ahead and done what I think is a more complete implementation of it, keeping generally to how you have architected this, but changing a few things from your partial implementation, which I describe in the PR. Open to all feedback. |
|
@notdaniel Thanks! I've used pub/sub on a single Redis server and I've used cluster mode, but I'd never combined them until recently and I realized it didn't work the way I expected. That's when I found out about the shard pub/sub commands. I've still never used them together in a real production environment, so there may still be edge cases I haven't considered. Any real-world experience on this is very much appreciated. I've skimmed through the code and it looks great so far. Thank you for adding server version requirements to the docs. I've only recently been tracking changes between server versions so, again, assistance there is always welcome. I'll take a closer look soon and test it against my cluster. |
|
@jgaskins No problem, happy to help. (I'm trying to port a big cluster streams-based analytics/event sourcing system at work over to crystal, so I would need to figure this out, anyway.) I mostly work with valkey cluster now, so I'm familiar with this. Needing sharded pubsub is a... more recent requirement, but it turns out that having 250 pods handling 75k publisher/subscribers with no shard-specific routing, just publishing things into the void, does not scale well. Shocker! (I found an instance the other day of a message that was published and picked up by around 25k subscribers when only 21 actually needed it.) Heads up, I'm also opening several issues for things I found elsewhere in the library when trying to get the cluster stuff integrated, but I will also fix some of them |
Add
SPUBLISHto the shared command API and exposeSSUBSCRIBEthrough the pooled client. Track ordinary, pattern, and sharded subscriptions separately soSubscription#closesends the correct unsubscribe command and consumes all pending acknowledgements.Remove cluster-level subscription methods that bypass command routing or operate on an unrelated pooled connection, and replace timing-based Pub/Sub specs with deterministic integration coverage.
Some of this changes how the partial implementation was done, so I'm explaining those decisions here:
Cluster#spublish; definedspublishonce insrc/commands.crreturningInt64The removed direct cluster implementation selected the correct initial pool, but it bypassed the broader redirection and recovery behavior in
Cluster#run, including handlingMOVED/ASK, standard logging, etc. It also duplicated command construction and left clients withoutspublish.sunsubscribemethodSubscriptions are connection-scoped.
Cluster#sunsubscribechecked out an arbitrary pooled connection, which was generally not the connection that owned the active target subscription.Subscriptionimplementation from one@channelsset forsubscribe,psubscribe, andssubscribeto three -- one each for ordinary channels, patterns, and shard channels.One set did not preserve enough information to issue the correct command when closing a subscription. This makes it explicit and is still rather simple.
Breaking on
argument == 0would leave subsequent acks from other channel types and desync the data. The tracked sets express the condition we actually want: no subscriptions of any kind remain.