Fix Reload topology only when changes occur (#2299) #3256
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- reload() (and updateCache) was always called even if the topology was unchanged, leading to unnecessary object allocations. Now, reload happens only when a genuine change is detected.
- Removed unnecessary Delay in tests. - Added a reload condition so that explicit calls to refreshPartitions() always trigger a reload. - Refactored refreshPartitionsAsync() to use method overloading to clearly separate explicit reload behavior from periodic refresh.
|
Hello, I have some questions regarding this change. Previously, a reload was only triggered when isChanged() returned true, so explicit calls to refreshPartitions() did not perform a reload. To address this, I modified the code using method overloading so that when refreshPartitions() is called explicitly, a reload always occurs. Could you please confirm whether this approach is acceptable? If there are any concerns with this implementation or if it doesn't align with the Lettuce team's direction, I'll be happy to re-evaluate the approach. Thank you. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This pull request addresses issue #2299. In the previous implementation, the reload() method was called even when no actual change occurred in the topology, which resulted in frequent invocations of updateCache() and unnecessary object allocations (such as new slotCache arrays). This fix ensures that reload() is executed only when a genuine topology change is detected via TopologyComparators.isChanged().
Changes
Conditional Reload:
Modified the logic in refreshPartitionsAsync() so that reload() (and consequently updateCache()) is only invoked when a change in the topology is observed.
Test Enhancements:
Added tests to compare scenarios:
When there is no topology change, the test verifies that reload is not triggered (i.e., updateCache is not called, and the update count remains 0).
When a simulated topology change occurs, the test confirms that reload is invoked at least once.
All cluster-related tests have been executed, and the results are as expected.
Testing
The tests leverage reflection to track the slotCache field changes in the Partitions class.
The objective is to ensure that:
In the absence of topology changes, no unnecessary reloads occur.
When a topology change is simulated, reload() correctly triggers a new allocation for the slotCache.
If additional tests or modifications are needed, I am happy to incorporate further refinements based on your feedback.
Final Notes
Thank you for reviewing this PR. I appreciate your consideration and any further suggestions for improvement.
Make sure that:
mvn formatter:formattarget. Don’t submit any formatting related changes.