-
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
fix(Cluster): don't fetch node info from the subscribe #697
Conversation
This fix will skip the subscriber who can't execute any command. Close #696
@luin I might be misunderstanding the code, but I'm pretty sure this doesn't skip the subscriber node, and only creates a debug message. |
@jeremytm you are correct |
@luin |
lib/cluster/index.js
Outdated
const node = _this.connectionPool.nodes.all[keys[index]] | ||
debug('getting slot cache from %s', key); | ||
if (_this.subscriber === node) { | ||
debug('skipping because the node is the subscriber'); |
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.
added
tryNode(index + 1);
return;
lib/cluster/index.js
Outdated
@@ -414,6 +414,9 @@ Cluster.prototype.refreshSlotsCache = function (callback) { | |||
debug('getting slot cache from %s', key); | |||
if (_this.subscriber === node) { | |||
debug('skipping because the node is the subscriber'); | |||
lastNodeError = new Error('Node is the subscriber'); |
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.
@luin
The problem occurs when querying the list of clusters, the attempt is from ioredis, is not an error, and makes it difficult to identify it when the actual error occurs.
Seeing similar things... what's the status on this? |
The previous solution solves the problem partially, though, a better way is to create a specialized connection for the subscription. I've updated this pull request with the new solution. Please take a look at it. |
2fd3d94
to
9582b8c
Compare
Previously (v3 & v4.0.0), ioredis reuse the existing connection for subscription, which will cause problem when executing commands on the reused connection. From now on, a specialized connection will be created when any subscription has made. This solves the problem above perfectly. Close #696
9582b8c
to
4c6ffeb
Compare
Have fully tested with socket.io-redis. Merged and released in v4.0.1. Feel free to report any issues in this. version |
This fix will skip the subscriber who can't execute any command.
Close #696