[#2341] Initialize slots with empty BitSet in RedisClusterNode's constructors

[zeze1004]

Issue: #2341
This PR supersedes the previously closed PR #2798

Make sure that:

• You have read the contribution guidelines.
• You have created a feature request first to discuss your contribution intent. Please reference the feature request ticket number in the pull request.
• You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
• You submit test cases (unit or integration tests) that back your changes.

[zeze1004]

@mp911de

I kindly request your review of this pull request. Your feedback would be greatly appreciated. 🙇🏻

[zeze1004]

I tested everything locally and it passed, but it failed on GitHub Actions. I'll make the necessary fixes and commit again soon.

[tishun]

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

[tishun]

Thanks for working on this!

Please check my comments and let me know if you need any clarifications or have some concerns.

[tishun]

Hey thanks for addressing the last comments.

There are still two things that I am worried about:

1. We are still missing the same logic on line 90 (the first of the three constructors)
2. The test is not really verifying the issue from RedisClusterNode.hasSameSlotsAs() is unreliable  #2341

[tishun]

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

[zeze1004]

I missed the part where we need to compare the original cluster node with the cloned object. Thanks for the feedback!

[zeze1004]

I made the changes based on your feedback. Can you please take a look? 🙏🏻

[mp911de]

Generally, we attempt to be as light-weight as possible. RedisClusterNode.slots is designed to be null so it would make sense to update both, the constructor of RedisClusterNode to expect a null slots argument and update ClusterPartitionParser to return a null slots bitset if the slotStrings would result in an empty slot specification (by also considering TOKEN_SLOT_IN_TRANSITION filtering, slotStrings.isEmpty() isn't sufficient).

Going forward, the parser for CLUSTER SHARDS in ClusterPartitionParser should be revisited as well to mimic the same behavior.

[tishun]

Generally, we attempt to be as light-weight as possible. RedisClusterNode.slots is designed to be null so it would make sense to update both, the constructor of RedisClusterNode to expect a null slots argument and update ClusterPartitionParser to return a null slots bitset if the slotStrings would result in an empty slot specification (by also considering TOKEN_SLOT_IN_TRANSITION filtering, slotStrings.isEmpty() isn't sufficient).

Going forward, the parser for CLUSTER SHARDS in ClusterPartitionParser should be revisited as well to mimic the same behavior.

Not a huge fan of using null myself as I tend to agree that this is the billion dollar mistake.

Perhaps a compromise solution would be a new BitSet(0) (Null object pattern)?
We could also make this object a singleton for improved memory consumption.

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?

[mp911de]

Allocation of BitSet uses up a bit of memory and CPU, hence we resorted to null. If you want to follow the null object pattern, then introducing a Slots object (backed by BitSet in the implemented variant) would rather make sense.

[zeze1004]

Hey everyone,

Creating a new Slots object to handle null slots sounds like a good idea.
However, I think using BitSet(0) when slots is null can also safely avoid NPEs.
Additionally, this approach would have less impact on the existing codebase compared to introducing a new Slots object.

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!

[tishun]

Allocation of BitSet uses up a bit of memory and CPU, hence we resorted to null. If you want to follow the null object pattern, then introducing a Slots object (backed by BitSet in the implemented variant) would rather make sense.

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.

[zeze1004]

I acknowledge the necessity of creating a separate Slots object. However, modifying the constructor of RedisClusterNode to include this new Slots class would require changes to all existing code that uses this constructor. This makes the scope of such a change too broad for this PR.

For this PR, I suggest we focus on the current approach of using BitSet with null checks. Implementing the Slots class could be considered in future updates through a gradual approach.
I will make the changes as reviewed by @tishun as soon as possible.

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

As English is not my first language, I would appreciate it if you could confirm whether I have correctly understood your opinions. Thank you.

[zeze1004]

I confirmed that my pull request failed during the integration tests. It seems like the failure is related to HashCommandIntegrationTests, which doesn’t seem to be directly connected to the changes I made. However, if there is an issue with my code, please let me know and I’ll fix it right away. cc. @tishun 🙇🏻

[tishun]

I confirmed that my pull request failed during the integration tests. It seems like the failure is related to HashCommandIntegrationTests, which doesn’t seem to be directly connected to the changes I made. However, if there is an issue with my code, please let me know and I’ll fix it right away. cc. @tishun 🙇🏻

There was a change in the redis/redis codebase that broke the tests, it has been since fixed.
I've started the pipeline again.

[tishun]

There was a change in the redis/redis codebase that broke the tests, it has been since fixed. I've started the pipeline again.

Actually - my bad - you will have to rebase your change to include the fix. Sorry about that.

See #2871

[zeze1004]

There was a change in the redis/redis codebase that broke the tests, it has been since fixed. I've started the pipeline again.

Actually - my bad - you will have to rebase your change to include the fix. Sorry about that.
See #2871

Thanks @tishun, I completed the rebase based on the PR you mentioned.