-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix(ssubscribe): Force slots refresh on MOVED error when using ssubscribe #1987
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
base: v4
Are you sure you want to change the base?
Conversation
59dd4d0
to
8f52163
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CLGTM
if (item.command.name == "ssubscribe" && err.message.includes("MOVED")) { | ||
this.redis.emit("moved"); | ||
return; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add a comment here explaining how we use the standalone client for ssubscibe
connections
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a hashslot is moved to another shard the message that is sent on the pubsub channel is sunsubscribe
, not moved
. I suspect this is working because it's caught when the connection is retried. On issuing the ssubscribe
command, the response is moved
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this again today and in fact there aren't any messages sent on the existing connection before it is closed. The connection is closed instantly during a failover, and then during retry, a MOVED
error is received on SSUBSCRIBE
as we assumed above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need tests on this. Please fix and enable the existing ones that weren't running before
When issuing a
ssubscribe
command, the Cluster client creates a "regular" Redis client to establish a connection to the correct shard. This doesn't work if there's a failover since the client doesn't know how to correctly handle aMOVED
response. This PR handles such events by propagating thessubscribe MOVED
error from theDataHandler
, through theClusterSubscriber
and theClusterSubscriberGroup
to theRedisCluster
client and forces slots refresh