-
Notifications
You must be signed in to change notification settings - Fork 228
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
Support preserving history of HELIX_ENABLED legacy fields when combo of instance operation and old helix version are used #2987
Support preserving history of HELIX_ENABLED legacy fields when combo of instance operation and old helix version are used #2987
Conversation
ea5ae20
to
78b5259
Compare
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.
Thanks for the change.
I would suggest separate the formatting and logic change for easier review. :D
helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java
Outdated
Show resolved
Hide resolved
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.
The pr is too large for reviewing combining format change + logic change. Please split the PR with purely logic change + refactoring.
helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java
Outdated
Show resolved
Hide resolved
helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java
Outdated
Show resolved
Hide resolved
Also, this is related to instance operations. Highly recommend to fix the unstable instance operation test as well. It is vulnerable there for a long while. |
…at an older version of helix could have been used to modify the legacy fields of HELIX_ENABLED, HELIX_DISABLED_TYPE, and HELIX_DISABLED_REASON. When this happens, we need to write those legacy fields to the USER source instance operation to preserve the history when another source sets the instance operation. When the other source re-enables the instance, we should restore the old legacy fields with what was previously set.
78b5259
to
f83072b
Compare
helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java
Outdated
Show resolved
Hide resolved
helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java
Outdated
Show resolved
Hide resolved
Overall looks good. Only one minor comment. |
This PR is ready to be merged. Commit message: |
…of instance operation and old helix version are used (apache#2987) When the new instance operation APIs are used, we need to consider that an older version of helix could have been used to modify the legacy fields of HELIX_ENABLED, HELIX_DISABLED_TYPE, and HELIX_DISABLED_REASON. When this happens, we will write those legacy fields to the USER source instance operation to preserve the history when another source sets the instance operation. When the other source re-enables the instance, we should restore the old legacy fields with what was previously set.
Issues
Description
When the new instance operation APIs are used, we need to consider that an older version of helix could have been used to modify the legacy fields of HELIX_ENABLED, HELIX_DISABLED_TYPE, and HELIX_DISABLED_REASON. When this happens, we need to write those legacy fields to the USER source instance operation to preserve the history when another source sets the instance operation. When the other source re-enables the instance, we should restore the old legacy fields with what was previously set.
Tests
Changes that Break Backward Compatibility (Optional)
Documentation (Optional)
(Link the GitHub wiki you added)
Commits
Code Quality
(helix-style-intellij.xml if IntelliJ IDE is used)