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

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

Merged
merged 6 commits into from
Jul 12, 2024

Conversation

zeze1004
Copy link
Contributor

@zeze1004 zeze1004 commented May 12, 2024

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
Copy link
Contributor Author

@mp911de

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

@zeze1004
Copy link
Contributor Author

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

@tishun
Copy link
Collaborator

tishun commented May 23, 2024

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

Copy link
Collaborator

@tishun tishun left a 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.

@zeze1004 zeze1004 force-pushed the fix/RedisClusterNode.hasSameSlotsAs() branch from 25d74c3 to 4fe8d21 Compare June 1, 2024 06:36
@zeze1004 zeze1004 requested a review from tishun June 1, 2024 07:17
Copy link
Collaborator

@tishun tishun left a 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:

  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

@mp911de
Copy link
Collaborator

mp911de commented Jun 6, 2024

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 tishun added the type: bug A general bug label Jun 6, 2024
@tishun tishun added this to the 7.x milestone Jun 6, 2024
@tishun
Copy link
Collaborator

tishun commented Jun 7, 2024

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
Copy link
Collaborator

mp911de commented Jun 7, 2024

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
Copy link
Contributor Author

zeze1004 commented Jun 7, 2024

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
Copy link
Collaborator

tishun commented Jun 7, 2024

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
Copy link
Contributor Author

zeze1004 commented Jun 14, 2024

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
Copy link
Contributor Author

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
Copy link
Collaborator

tishun commented Jun 20, 2024

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
Copy link
Collaborator

tishun commented Jun 23, 2024

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
Copy link
Contributor Author

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.

@zeze1004 zeze1004 requested a review from tishun June 23, 2024 15:47
@tishun
Copy link
Collaborator

tishun commented Jul 2, 2024

Hm something seems off to me, your change should not span 85 files
I think something went wrong with the merge

@tishun tishun force-pushed the fix/RedisClusterNode.hasSameSlotsAs() branch from d873986 to 18572c0 Compare July 12, 2024 10:48
@tishun
Copy link
Collaborator

tishun commented Jul 12, 2024

Hm something seems off to me, your change should not span 85 files I think something went wrong with the merge

There, I fixed it :)

@tishun tishun merged commit dee8020 into redis:main Jul 12, 2024
5 checks passed
@zeze1004 zeze1004 deleted the fix/RedisClusterNode.hasSameSlotsAs() branch September 1, 2024 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants