Skip to content
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

Merged
merged 4 commits into from
Oct 8, 2018

Conversation

luin
Copy link
Collaborator

@luin luin commented Sep 14, 2018

This fix will skip the subscriber who can't execute any command.

Close #696

This fix will skip the subscriber who can't execute
any command.

Close #696
@jeremytm
Copy link

@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.

@shaharmor
Copy link
Collaborator

@jeremytm you are correct

@hhgyu
Copy link

hhgyu commented Sep 21, 2018

@luin
#697 (diff)
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.

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');
Copy link

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;

@@ -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');
Copy link

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.

@stockholmux
Copy link

Seeing similar things... what's the status on this?

@luin
Copy link
Collaborator Author

luin commented Oct 6, 2018

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.

@luin luin force-pushed the fix/cluster-refresh branch 9 times, most recently from 2fd3d94 to 9582b8c Compare October 6, 2018 17:42
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
@luin luin merged commit 13a5bc4 into master Oct 8, 2018
@luin luin deleted the fix/cluster-refresh branch October 8, 2018 04:55
@luin
Copy link
Collaborator Author

luin commented Oct 8, 2018

Have fully tested with socket.io-redis. Merged and released in v4.0.1. Feel free to report any issues in this. version

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refreshing nodes on interval timer breaks socket.io pubsub adapter
5 participants