-
Notifications
You must be signed in to change notification settings - Fork 237
Add check for customized resources in isEvacuatedFinished #3023
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
base: master
Are you sure you want to change the base?
Add check for customized resources in isEvacuatedFinished #3023
Conversation
Thanks for contributing to open source!
|
Merge master into dev
InstanceConfig instanceConfig = cache.getInstanceConfigMap().get(instance); | ||
boolean isInstanceEvacuated = instanceConfig != null && | ||
instanceConfig.getInstanceOperation().getOperation() == InstanceConstants.InstanceOperation.EVACUATE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we rename the boolean field toisAssignableForCustomizedResource
? This will make it more extensible for future use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that when an instance is offline but tagged EVACUATE, its partition isn’t dropped. This seems unintended because we should make it be consistent with the old behavior. Could we add a check to ensure nodes marked EVACUATE are live?
isAssignableForCustomizedResource = isLive && hasEvacuateTag
- Add isInstanceDrained method in HelixAdmin - Expose the method via instance update rest end point - Change the conditional checks order in isEvacuateFinished to improve latency
24bd881
to
86abe3f
Compare
InstanceConfig instanceConfig = cache.getInstanceConfigMap().get(instance); | ||
boolean hasEvacuatedOp = instanceConfig != null && | ||
instanceConfig.getInstanceOperation().getOperation() == InstanceConstants.InstanceOperation.EVACUATE; | ||
boolean isAssignableForCustomizedResource = cache.getLiveInstances().containsKey(instance) && hasEvacuatedOp; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we leave custom rebalancer away with this logic currently? I think this requires a comprehensive design with custom rebalance users in open source.
Issues
(#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
isInstanceDrained
to check if the instance doesn't has any resource with rebalance mode full auto or customized in the current stateisInstanceDrained
check via API(Write a concise description including what, why, how)
Tests
The following tests are written for this issue:
TestInstanceOperation.testEvacuateWithCustomizedResource()
TestCustomRebalancer.testDisabledBootstrappingPartitions()
have been adjusted appropriatelyThe following is the result of the "mvn test" command on the appropriate module:
(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)