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

Address empty cluster slots list when discovering slots #3195

Merged
merged 11 commits into from
Nov 17, 2022

Conversation

jgaoIXL
Copy link
Contributor

@jgaoIXL jgaoIXL commented Nov 12, 2022

This is a bug that I encountered where renewClusterSlots was called when one node was removed from the cluster. The removed node happened to get selected to do the discoverClusterSlots. Because the removed node return no cluster slots, we have an empty slots cache, and there is no recovery possible from there since the nodes and slots map are all empty.

I think it makes sense to return if slotsInfo is size 0 because under no normal circumstances, the cluster should have 0 cluster info provided, it can only mean one thing, which is you are pointing to a node that is not in the cluster.

zeekling
zeekling previously approved these changes Nov 13, 2022
Copy link
Contributor

@zeekling zeekling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems reasonable.

@sazzad16 sazzad16 added this to the 4.4.0 milestone Nov 13, 2022
Copy link
Contributor

@sazzad16 sazzad16 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use isEmpty() method.

@jgaoIXL jgaoIXL dismissed stale reviews from zeekling and ghost via c44d2f7 November 14, 2022 17:48
@jgaoIXL
Copy link
Contributor Author

jgaoIXL commented Nov 14, 2022

Let's use isEmpty() method.

Fixed, also discoverClusterNodesAndSlots should probably throw an exception when this happens.

@sazzad16 sazzad16 changed the title Fix discover slots Address empty cluster slots list when discovering slots Nov 15, 2022
@yangbodong22011
Copy link
Collaborator

@jgaoIXL Jedis assumes that the initial node of the cluster is constant (that is, it always belongs to this cluster),whether it is vip, domain, or physical ip.

But I agree to merge this PR, as a defensive protection.

@redis redis deleted a comment from yangbodong22011 Nov 16, 2022
@sazzad16
Copy link
Contributor

@yangbodong22011 I've mistakenly deleted one of your comment/suggestion. Sorry about that.

Copy link
Contributor

@sazzad16 sazzad16 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exception should be JedisClusterOperationException. WIP #3204

@sazzad16 sazzad16 merged commit af06de8 into redis:master Nov 17, 2022
@jgaoIXL
Copy link
Contributor Author

jgaoIXL commented Nov 17, 2022

Thank you. When are we supposed to see a stable release with this fix? @yangbodong22011

@sazzad16
Copy link
Contributor

@jgaoIXL We don't have an exact date yet but this change is available in 4.4.0 snapshot.

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

Successfully merging this pull request may close these issues.

4 participants