Fix is Instance enabled #2972
Fix is Instance enabled #2972
Conversation
| // it is not likely that instanceEnabled is unset, because when we set operation to disable, | ||
| // we always set instanceEnabled to false |
There was a problem hiding this comment.
Could you please clarify the comments? Does 'InstanceEnabled' refer to 'HELIX_ENABLED'?
| // If instance is enabled by old version helix (not having instance operation), the instance config | ||
| // will have HELIX_ENABLED set to true. In this case, we should override the instance operation to ENABLE | ||
| if ("true".equals(_record.getSimpleField(InstanceConfigProperty.HELIX_ENABLED.name()))) { | ||
| return new InstanceOperation.Builder().setOperation(InstanceConstants.InstanceOperation.ENABLE).build(); |
There was a problem hiding this comment.
good point. But it is a bit hard to write back. We do not have access to ZK client or data accessor here. It adds too much complexity. As long as all new/old reader honor the HELIX_ENABLED then I think we are good.
There was a problem hiding this comment.
I think it might be dangerous to write back as client may update the config while we write back. It could overwrite the current value. (Unlike the batch enable/disable one as that field is deprecated and we just need to clean it out)
Since the getter will use the HELIX_ENABLED and is compatible with both new/old client before instance operation, I think it is safe to keep it the current way.
|
This change is approved by @junkaixue |
Issues
#2975
Description
Helix deprecated the HELIX_ENABLED and added a new InstanceOperation field.
If an instance is disabled using InstanceOperation but enabled using setting HELIX_ENABLED to true (by helix lib with old version), IsInstanceEnabled will return false.
When HELIX_ENABLED is true, but InstanceOperation is DISABLED, IsInstanceEnabled returns false. Where it should return true.
Tests
testEnabledInstanceBackwardCompatibility
(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)