Closed zeze1004 closed 2 months ago
@mp911de
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.
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
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.
Generally, we attempt to be as light-weight as possible.
RedisClusterNode.slots
is designed to benull
so it would make sense to update both, the constructor ofRedisClusterNode
to expect anull
slots argument and updateClusterPartitionParser
to return anull
slots bitset if theslotStrings
would result in an empty slot specification (by also consideringTOKEN_SLOT_IN_TRANSITION
filtering,slotStrings.isEmpty()
isn't sufficient).Going forward, the parser for
CLUSTER SHARDS
inClusterPartitionParser
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?
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.
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!
Allocation of
BitSet
uses up a bit of memory and CPU, hence we resorted tonull
. If you want to follow the null object pattern, then introducing aSlots
object (backed byBitSet
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.
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.
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 🙇🏻
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.
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.
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 https://github.com/redis/lettuce/pull/2871
Thanks @tishun, I completed the rebase based on the PR you mentioned.
Hm something seems off to me, your change should not span 85 files I think something went wrong with the merge
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 :)
Issue: #2341 This PR supersedes the previously closed PR #2798
Make sure that:
[x] You submit test cases (unit or integration tests) that back your changes.