-
Notifications
You must be signed in to change notification settings - Fork 945
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
[#2341] Initialize slots with empty BitSet in RedisClusterNode's constructors #2852
base: main
Are you sure you want to change the base?
[#2341] Initialize slots with empty BitSet in RedisClusterNode's constructors #2852
Conversation
…de's constructors
…terNode constructors
I kindly request your review of this pull request. Your feedback would be greatly appreciated. 🙇🏻 |
I tested everything locally and it passed, but it failed on GitHub Actions. I'll make the necessary fixes and commit again soon. |
Please ignore the failure of the SslIntegrationTests.pubSubSsl:396 - it is unstable and if you pull the latest sources you will see it is disabled |
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.
Thanks for working on this!
Please check my comments and let me know if you need any clarifications or have some concerns.
@@ -103,8 +103,7 @@ public RedisClusterNode(RedisURI uri, String nodeId, boolean connected, String s | |||
this.configEpoch = configEpoch; | |||
this.replOffset = -1; | |||
|
|||
this.slots = new BitSet(slots.length()); | |||
this.slots.or(slots); | |||
this.slots = slots != null ? slots : new BitSet(0); |
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.
I think part of the idea behind the existing code is to clone the content of the slots, instead of directly assigning the provided value.
This has the added benefit of isolation, because the code that called the constructor could no longer modify the contents of the slot (as a side effect).
However you are correct that a good design should consider this method receiving a null for the slots argument. Perhaps something in the line of:
this.slots = new BitSet(SlotHash.SLOT_COUNT);
this.slots.or(slots);
This way:
- if the slots argument are null then the
or()
method handles it correctly - the
this.slots
is always initialized with an empty list of slots
IMHO the consistency of always having BitSet initialized with the SlotHash.SLOT_COUNT
pays off for the "waste" of memory.
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.
@tishun Thank you for your feedback. I have made the suggested changes.
@@ -123,8 +122,9 @@ public RedisClusterNode(RedisClusterNode redisClusterNode) { | |||
this.replOffset = redisClusterNode.replOffset; | |||
this.aliases.addAll(redisClusterNode.aliases); | |||
|
|||
if (redisClusterNode.slots != null && !redisClusterNode.slots.isEmpty()) { | |||
this.slots = new BitSet(SlotHash.SLOT_COUNT); | |||
this.slots = redisClusterNode.slots != null ? new BitSet(SlotHash.SLOT_COUNT) : new BitSet(0); |
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.
I think the same logic can be followed here. See my previous comment.
|
||
assertThat(copiedNode.getSlots()).containsExactly(1, 2); | ||
} | ||
|
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.
I think the most important test is the one described in #2341 and it is missing:
- create one RedisClusterNode instance from the constructor on line 102-103 (parsed nodes use this constructor)
- create one RedisClusterNode instance from the constructor on line 112-125 (cloned nodes use this constructor)
- check if
RedisClusterNode::hasSameSlotsAs()
returns that they are the same
If you see the last mention of an issue in the original issue you'll see that there is also a 3rd constructor (the one on line 78-79) that calls the setSlotBits()
method and it would also result in an null slots
field if the array is empty. If we had a test for that we would have caught that this condition is not handled right now.
I think a good design would address this and make sure there is no way we set null to the slots field. What do you think?
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.
Thank you. I forgot to add the necessary test. I've added it. Could you please review again?
25d74c3
to
4fe8d21
Compare
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.
Hey thanks for addressing the last comments.
There are still two things that I am worried about:
- We are still missing the same logic on line 90 (the first of the three constructors)
- The test is not really verifying the issue from RedisClusterNode.hasSameSlotsAs() is unreliable #2341
} | ||
|
||
@Test | ||
public void testHasDifferentSlotsAs() { |
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.
This test is OK, but it tests something different than what is explained in #2341
In #2341 the problems is that one part of the logic uses RedisClusterNode(RedisURI uri, String nodeId, boolean connected, String slaveOf, long pingSentTimestamp, long pongReceivedTimestamp, long configEpoch, BitSet slots, Set<NodeFlag> flags)
to create a RedisClusterNode, while another uses the public RedisClusterNode(RedisClusterNode redisClusterNode)
constructor. Both are provided an empty BitSet.
When later on both resulting objects are compared with .hasSameSlotsAs()
it would return false
, when it should return true
, because the first object would have an empty slots
BitSet, while the second would have a null slots
BitSet.
I think the proper test scenario is :
public void testHasDifferentSlotsAs() {
BitSet emptySlots = new BitSet(SlotHash.SLOT_COUNT);
RedisClusterNode node1 = new RedisClusterNode(RedisURI.create("localhost", 6379), "nodeId1", true, "slaveOf", 0L, 0L,
0L, emptySlots, new HashSet<>());
RedisClusterNode node2 = new RedisClusterNode(node1);
assertThat(node1.hasSameSlotsAs(node2)).isTrue();
You can test if this case fails when you remove your changes to RedisClusterNode.java
Generally, we attempt to be as light-weight as possible. Going forward, the parser for |
Not a huge fan of using Perhaps a compromise solution would be a It would still be more than what null would take, but it would be insignificantly more. In the same time it would have the benefit of avoiding NPEs and not having to deal with null checks. What's your take on that approach? |
Allocation of |
Hey everyone, Creating a new Slots object to handle null slots sounds like a good idea. That said, if most contributors, including @tishun, prefer creating the Slots object, I’m ready to make the changes accordingly. Thanks for all the reviews and feedback! |
The memory allocated (retained size) for a singleton BitSet with size 0 is 40B total (24B of which is shallow size). For a singleton BitSet the CPU overhead would be only once per creation. IMO these are negligible and indistinguishable from using null for any modern hardware and JVM. I was going to propose to create a dedicated object myself, but would that affect the public API? My recommendation would be to go with the null object, but I will approve any solution that resolves the problem without introducing new problems itself. |
Issue: #2341
This PR supersedes the previously closed PR #2798
Make sure that: