-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Sharded pub/sub support via dedicated subscribers #1956
base: main
Are you sure you want to change the base?
Conversation
* @param node | ||
* @param readOnly | ||
*/ | ||
createRedisFromOptions(node: RedisOptions, readOnly: boolean) { |
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.
What do you think about adding a useShardedPubsub
option? It would be false
by default, and would control whether we eagerly initialise the new pool of socket 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.
I think you are right. The way how it is implemented runs now one subscriber connection per shard. Which doubles the number of connections to a cluster. I added an option called "shardedSubscribers".
BTW: This especially relevant for RESP2. The library doesn't support RESP3 yet. Later when RESP3 might be supported, users can then decide if they want to subscribe on the normal connections or if they want to use dedicated connections for the sharded subscriptions.
I added a new configuration option called shardedSubscribers
, which defaults to false
.
*/ | ||
refreshSlots(cluster: Cluster) : boolean { | ||
//If there was an actual change, then reassign the slot ranges | ||
if (this.clusterSlots !== cluster.slots) { |
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.
Since !==
only compares references, we should add a test to verify the refresh logic and confirm whether we need a deep equality check using JSON.stringify
.
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.
Yes, I will investigate that.
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.
OK, added a deep equality check via JSON.stringify.
There are some tests that need attention as well:
|
… cluster before doing a refresh
The previous implementation used one subscriber connection per cluster instance, but the command
SSUBSCRIBE <channel>
needs to be executed against a node that is responsible for the hash slot that is derived from the channel name.This PR adds a ClusterSubscriberGroup whereby each subscriber in the group is responsible for one shard.