Skip to content
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.

Sign up for GitHub

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

Fix is Instance enabled #2972

Merged
merged 4 commits into from
Dec 5, 2024
Merged

Fix is Instance enabled #2972

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7391e6e
fix enabled
xyuanlu Nov 26, 2024
4ba1077
add test
xyuanlu Dec 2, 2024
e338f3d
comments
xyuanlu Dec 3, 2024
bdfab05
edit typo in comment
xyuanlu Dec 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -642,8 +642,8 @@ public InstanceOperation getInstanceOperation() {
.build();
}

// Always respect the HELIX_ENABLED being set to false when instance operation is unset
// for backwards compatibility.
// if instance operation is not DISABLED, we always respect the HELIX_ENABLED being set to false
// when instance operation is unset for backwards compatibility.
if (!_record.getBooleanField(InstanceConfigProperty.HELIX_ENABLED.name(),
HELIX_ENABLED_DEFAULT_VALUE)
&& (InstanceConstants.INSTANCE_DISABLED_OVERRIDABLE_OPERATIONS.contains(
Expand All @@ -656,6 +656,17 @@ public InstanceOperation getInstanceOperation() {
.build();
}

// if instance operation id DISABLE, we override it to ENABLE if instance is enabled
if (activeInstanceOperation.getOperation().equals(InstanceConstants.InstanceOperation.DISABLE)) {
// it is not likely that instanceEnabled is unset, because when we set operation to disable,
// we always set instanceEnabled to false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we write it back?

Copy link
Contributor Author

@xyuanlu xyuanlu Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

return activeInstanceOperation;
}

Expand Down