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

RedisClusterNode.hasSameSlotsAs() is unreliable #2341

Closed
jacob-pro opened this issue Feb 28, 2023 · 3 comments
Closed

RedisClusterNode.hasSameSlotsAs() is unreliable #2341

jacob-pro opened this issue Feb 28, 2023 · 3 comments
Labels
status: good-first-issue An issue that can only be worked on by brand new contributors type: bug A general bug

Comments

@jacob-pro
Copy link
Contributor

jacob-pro commented Feb 28, 2023

Bug Report

Current Behavior

When using the RedisClusterNode::hasSameSlotsAs() function to compare nodes retrieved from RedisClusterClient::getPartitions() against nodes from ClusterPartitionParser::parse (via RedisClusterCommands::clusterNodes) it incorrectly returns false.

This is because the nodes in the client partitions table have a slot range of null whereas the parsed nodes have a non null but empty slot range.

The parsed nodes are created with a non-null BitSet even if they have no slots:

https://github.com/lettuce-io/lettuce-core/blob/fc6630e0fdd46e175e1d51a2491732092bb683ff/src/main/java/io/lettuce/core/cluster/models/partitions/RedisClusterNode.java#L102-L103

Whereas cloned nodes are created with a null BitSet if the slots are empty:

https://github.com/lettuce-io/lettuce-core/blob/fc6630e0fdd46e175e1d51a2491732092bb683ff/src/main/java/io/lettuce/core/cluster/models/partitions/RedisClusterNode.java#L122-L125

Updating the slots also leaves the value as null if the new range is empty:

https://github.com/lettuce-io/lettuce-core/blob/fc6630e0fdd46e175e1d51a2491732092bb683ff/src/main/java/io/lettuce/core/cluster/models/partitions/RedisClusterNode.java#L332-L334

Environment

  • Lettuce version(s): 6.2.3.RELEASE
  • Redis version: 7.0.8

Additional context

Similar / related to:

#1091
#1089

@mp911de mp911de added the type: bug A general bug label Apr 18, 2023
@mp911de
Copy link
Collaborator

mp911de commented Apr 18, 2023

It makes sense to fix this problem. Since you've looked already into it, do you want to submit a pull request along with a few unit test cases?

@mp911de mp911de added the status: good-first-issue An issue that can only be worked on by brand new contributors label Dec 13, 2023
@zeze1004
Copy link
Contributor

zeze1004 commented Feb 8, 2024

@mp911de , @jacob-pro
May I take on this issue?
I think it would work to have nodes without slots hold an empty BitSet object(or null) and modify the conditional statement based on #1091 issue.

I really want to give it a try!

@zeze1004
Copy link
Contributor

zeze1004 commented Feb 8, 2024

On second thought🤔🤔🤔, don't need to check the conditions to assigned a slot when creating a node. Just check if it's slot.empty().

What do you think of my opinion? Please review it.🙇🏻🙇🏻🙇🏻

tishun pushed a commit to zeze1004/lettuce-core that referenced this issue Jul 12, 2024
tishun pushed a commit to zeze1004/lettuce-core that referenced this issue Jul 12, 2024
tishun pushed a commit to zeze1004/lettuce-core that referenced this issue Jul 12, 2024
tishun pushed a commit to zeze1004/lettuce-core that referenced this issue Jul 12, 2024
tishun pushed a commit to zeze1004/lettuce-core that referenced this issue Jul 12, 2024
tishun pushed a commit to zeze1004/lettuce-core that referenced this issue Jul 12, 2024
…nstructor and compare the two clusters with hasSameSlotsAs()
tishun pushed a commit that referenced this issue Jul 12, 2024
…tructors (#2852)

* fix(#2341): Initialize slots with empty BitSet in RedisClusterNode's constructors

* test(#2341): Add test cases for slot initialization in RedisClusterNode constructors

* fix(#2341): Initialize RedisClusterNode slots with SlotHash.SLOT_COUNT

* chore(#2341): Adjust the formatting

* test(#2341):Add test cases for hasSameSlotsAs()

* fix(#2341): Clone node2 from node1 using the RedisClusterNode constructor and compare the two clusters with hasSameSlotsAs()
@tishun tishun closed this as completed Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: good-first-issue An issue that can only be worked on by brand new contributors type: bug A general bug
Projects
None yet
Development

No branches or pull requests

4 participants