Skip to content

cluster: make ForEachNode iterate through each node #703

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

Closed
wants to merge 1 commit into from

Conversation

gbbr
Copy link
Contributor

@gbbr gbbr commented Jan 31, 2018

ForEachNode was iterating through the masters and slaves fields,
which are the result of the addresses advertised by the cluster, but do
not contain the addresses used to create the cluster (in
ClusterOptions). This was problematic in some situations. This PR
addresses that issue.

This might make more sense because it is the nodes property that is used
when selecting nodes, not masters and slaves. (see here).

This addresses #701. If you decide against making this change, please
feel free to close the PR.

ForEachNode was iterating through the `masters` and `slaves` fields,
which are the result of the addresses advertised by the cluster, but do
not contain the addresses used to create the cluster (in
ClusterOptions). This was problematic in some situations. This PR
addresses that issue.
@vmihailenco
Copy link
Collaborator

I need to think about this, but I am not comfortable that with this change ForEachMaster+ForEachSlave != ForEachNode.

@gbbr
Copy link
Contributor Author

gbbr commented Feb 2, 2018

No problem, take your time. You have the context knowledge.

The way I thought of it was that ForEachMaster and ForEachSlave would iterate through the cluster advertised masters and slaves, while ForEachNode would iterate through all nodes including the ones used at startup time.

Either way, if you decide against it, I can work around it but in some situations it could be problematic given our scenario.

Thanks for looking.

@vmihailenco
Copy link
Collaborator

After some testing I think it should be ForEachNode = ForEachMaster + ForEachSlave. Otherwise it would require go-redis to keep connection/client to all nodes that ever existed in the cluster which sounds dubious.

@gbbr gbbr deleted the nodes branch February 15, 2018 11:05
@gbbr
Copy link
Contributor Author

gbbr commented Feb 15, 2018

Sure. I can definitely understand your point of view. And I'm ok with it 👍 I think my case is somewhat of an edge-case that should normally not happen in a real life scenario. I suspect my case was one of a misconfigured cluster, where the nodes addresses used with go-redis were different than the ones advertised by the cluster.

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.

2 participants