Add support to enable top state downward ST message as recovery rebalance #67
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
ngngwr
reviewed
Nov 4, 2025
| RebalanceType rebalanceType = | ||
| getRebalanceTypePerMessage(requiredState, message, derivedCurrentStateMap); | ||
|
|
||
| if(recoveryRebalanceForTopStateDownwardTransition) { |
Collaborator
There was a problem hiding this comment.
You can move the logic to getRebalanceTypePerMessage() method itself - instead of overriding the decision here.
| if (secondTopState == null) { | ||
| return false; | ||
| } | ||
| return msg.getFromState() == topState && msg.getToState() == secondTopState; |
Collaborator
There was a problem hiding this comment.
Suggested change
| return msg.getFromState() == topState && msg.getToState() == secondTopState; | |
| return topState.equals(msg.getFromState()) && secondTopState.equals(msg.getToState()); |
This might not work. We should use .equals for String comparision. IIRC == compares object reference and not the content.
| return getParticipantDeregistrationTimeout() > -1; | ||
| } | ||
|
|
||
| public boolean isRecoveryBalanceForTopStateDownwardTransitionEnabled() { |
Collaborator
There was a problem hiding this comment.
Suggested change
| public boolean isRecoveryBalanceForTopStateDownwardTransitionEnabled() { | |
| public boolean isRecoveryRebalanceForTopStateDownwardTransitionEnabled() { |
Rebalance**
| return _record.getBooleanField(ClusterConfigProperty.ENABLE_RECOVERY_REBALANCE_FOR_TOPSTATE_DOWNWARD_TRANSITION.name(), false); | ||
| } | ||
|
|
||
| public void setRecoveryBalanceForTopStateDownwardTransitionEnabled(boolean enabled) { |
Collaborator
There was a problem hiding this comment.
Suggested change
| public void setRecoveryBalanceForTopStateDownwardTransitionEnabled(boolean enabled) { | |
| public void setRecoveryRebalanceForTopStateDownwardTransitionEnabled(boolean enabled) { |
Rebalance**
| } | ||
|
|
||
| public void setRecoveryBalanceForTopStateDownwardTransitionEnabled(boolean enabled) { | ||
| _record.setBooleanField(ClusterConfigProperty.ENABLE_RECOVERY_REBALANCE_FOR_TOPSTATE_DOWNWARD_TRANSITION.name(),enabled); |
Collaborator
There was a problem hiding this comment.
Suggested change
| _record.setBooleanField(ClusterConfigProperty.ENABLE_RECOVERY_REBALANCE_FOR_TOPSTATE_DOWNWARD_TRANSITION.name(),enabled); | |
| _record.setBooleanField(ClusterConfigProperty.ENABLE_RECOVERY_REBALANCE_FOR_TOPSTATE_DOWNWARD_TRANSITION.name(), enabled); |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Issues
Presently any top state downward transition message from MASTER->SLAVE or (LEADER->STANDBY) gets evaluated as a load balance message. This is because when determining message type helix checks how many present participants are SLAVE and if that is >= min active required number(almost always 1), it treats the top state downward transition message MASTER->SLAVE as a load balance because it is increasing the number of slave replicas.
This causes an issue when it is urgently required to transfer the topstate and the load balance messages are throttled due to ST throttle configs.
(apache#200 - Link your issue number here: You can write "Fixes #XXX". Please use the proper keyword so that the issue gets closed automatically. See https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue
Any of the following keywords can be used: close, closes, closed, fix, fixes, fixed, resolve, resolves, resolved)
Description
Added a new config
ENABLE_RECOVERY_REBALANCE_FOR_TOPSTATE_DOWNWARD_TRANSITIONto handle such cases. In Intermediate State calc if the above config is true we treat any top state downward ST message as RECOVERY_REBALANCE.(Write a concise description including what, why, how)
Tests
TestIntermediateStateCalcStage.testEnableRecoveryRebalanceForTopStateDownwardStateTransition
(List the names of added unit/integration tests)
(If CI test fails due to known issue, please specify the issue and test PR locally. Then copy & paste the result of "mvn test" to here.)
Changes that Break Backward Compatibility (Optional)
(Consider including all behavior changes for public methods or API. Also include these changes in merge description so that other developers are aware of these changes. This allows them to make relevant code changes in feature branches accounting for the new method/API behavior.)
Documentation (Optional)
(Link the GitHub wiki you added)
Commits
Code Quality
(helix-style-intellij.xml if IntelliJ IDE is used)