search

vert-x3 / vertx-redis-client

Redis client for Vert.x
http://vertx.io
Apache License 2.0
131 stars 119 forks source link

Unexpected behavior of UNSUBSCRIBE with cluster mode #399

Closed jabolina closed 7 months ago

jabolina commented 1 year ago

Version

Reproduced with 4.4.4 and main.

Context

I noticed that an UNSUBSCRIBE command is issued for each slot individually in cluster mode. I believe this is not the expected behavior, just a single unsubscribe would be enough, but I couldn't find something clear on the specification regarding pub-sub + cluster mode.

From a debug, it seems the problem (if it is a problem), is because UNSUBSCRIBE is added to the REDUCERS:

https://github.com/vert-x3/vertx-redis-client/blob/bd32f71a411ecf6f12738885aed3dbcfaa65471d/src/main/java/io/vertx/redis/client/impl/RedisClusterClient.java#L120

When sending a command, if it is on the REDUCERS, it is sent for each slot:

https://github.com/vert-x3/vertx-redis-client/blob/bd32f71a411ecf6f12738885aed3dbcfaa65471d/src/main/java/io/vertx/redis/client/impl/RedisClusterConnection.java#L137-L156

Do you have a reproducer?

Created a small reproducer in this commit: https://github.com/vert-x3/vertx-redis-client/compare/master...jabolina:pubsub-cluster-unsub.

Steps to reproduce

  1. The test included in the commit will fail because the counter is decremented more than 3x.
Ladicek commented 7 months ago

I think this is intentional. When we SUBSCRIBE, we don't know to which node we're connected, it is selected randomly. So when we want to UNSUBSCRIBE, we need to execute that command on all nodes.

jabolina commented 7 months ago

Yeah, that makes sense to me, a single request to each node. I believe that currently, it sends N times for the same node, where N is the number of slots a node owns.

It looks like in cluster mode from version 7+, pub/sub goes to a slot owner (https://redis.io/docs/interact/pubsub/#sharded-pubsub).

Ladicek commented 7 months ago

Sharded pub/sub is done using SSUBSCRIBE, SUNSUBSCRIBE and SPUBLISH, so that's a different thing. What you have is not sharded. (I recently made a change where the sharded pub/sub commands are treated similarly to classic pub/sub, which was probably a mistake I made without understanding sharded pub/sub properly. I will have to revisit it.)

It seems super unlikely that we would send the command once for each slot, because the number of slots in Redis cluster is 16K. In the reproducing test you made, the cluster has 3 nodes, so I would expect the UNSUBSCRIBE command to be send 3x and hence there would be 3 responses to it. The total number of received messages in the test should IMHO be 5.

jabolina commented 7 months ago

Yeah, that's it. Coming back to the test now:

received msg [subscribe, sub-unsub, 1]
received msg [message, sub-unsub, hello]
received msg [unsubscribe, sub-unsub, 0]
received msg [unsubscribe, sub-unsub, 0]
received msg [unsubscribe, sub-unsub, 0]

One for each node. I believe we can safely close this one. Thanks for the help, and sorry about the noise.

Ladicek commented 7 months ago

Sure thing, thanks for the confirmation!