zpinto commented on code in PR #2987:
URL: https://github.com/apache/helix/pull/2987#discussion_r1920910604
##########
helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java:
##########
@@ -555,31 +574,68 @@ public void setInstanceOperation(InstanceOperation
operation) {
setInstanceEnabledHelper(false, operation.getTimestamp());
}
-
_record.setSimpleField(InstanceConfigProperty.HELIX_DISABLED_REASON.name(),
- operation.getReason());
- _record.setSimpleField(InstanceConfigProperty.HELIX_DISABLED_TYPE.name(),
-
InstanceConstants.InstanceDisabledType.DEFAULT_INSTANCE_DISABLE_TYPE.name());
+ resetInstanceDisabledTypeAndReason();
+ setInstanceDisabledReason(operation.getReason());
+ setInstanceDisabledType(operation.getLegacyDisabledType());
} else if (operation.getOperation() ==
InstanceConstants.InstanceOperation.ENABLE) {
- // If any of the other InstanceOperations are of type DISABLE, set that
in the HELIX_ENABLED,
- // HELIX_DISABLED_REASON, and HELIX_DISABLED_TYPE fields.
- InstanceOperation latestDisableInstanceOperation = null;
- for (InstanceOperation instanceOperation : getInstanceOperations()) {
- if (instanceOperation.getOperation() ==
InstanceConstants.InstanceOperation.DISABLE && (
- latestDisableInstanceOperation == null ||
instanceOperation.getTimestamp()
- > latestDisableInstanceOperation.getTimestamp())) {
- latestDisableInstanceOperation = instanceOperation;
+ // Ensure HELIX_ENABLED reflects the latest disable operation if
applicable.
+ InstanceOperation latestDisableInstanceOperation =
getInstanceOperations().stream()
+ .filter(op -> op.getOperation() ==
InstanceConstants.InstanceOperation.DISABLE)
+
.max(Comparator.comparingLong(InstanceOperation::getTimestamp)).orElse(null);
+
+ if (!_record.getBooleanField(InstanceConfigProperty.HELIX_ENABLED.name(),
+ HELIX_ENABLED_DEFAULT_VALUE)) {
+ // Only set the disable reason and disable type if the HELIX_ENABLED
is false because HELIX_ENABLED
+ // being true takes precedence over an existing latest disable
operation existing.
+ if (latestDisableInstanceOperation != null) {
+
setInstanceDisabledReason(latestDisableInstanceOperation.getReason());
+
setInstanceDisabledType(latestDisableInstanceOperation.getLegacyDisabledType());
+ } else {
+ setInstanceEnabledHelper(true, operation.getTimestamp());
}
}
+ }
+ }
- if (latestDisableInstanceOperation != null) {
-
_record.setSimpleField(InstanceConfigProperty.HELIX_DISABLED_REASON.name(),
- latestDisableInstanceOperation.getReason());
-
_record.setSimpleField(InstanceConfigProperty.HELIX_DISABLED_TYPE.name(),
-
InstanceConstants.InstanceDisabledType.DEFAULT_INSTANCE_DISABLE_TYPE.name());
- } else {
- setInstanceEnabledHelper(true, operation.getTimestamp());
- }
+ private void setInstanceOperationHelper(InstanceOperation operation) {
+ List<InstanceOperation> operations = getInstanceOperations();
+
+ if (operation.getSource() ==
InstanceConstants.InstanceOperationSource.ADMIN) {
+ operations.clear();
+ } else {
+ // Remove existing operation with the same source.
+ operations.removeIf(op -> op.getSource() == operation.getSource());
+ }
+
+ insertInstanceOperation(operations, operation);
+
+ // Serialize and update ZnRecord.
+
_record.setListField(InstanceConfigProperty.HELIX_INSTANCE_OPERATIONS.name(),
+ serializeInstanceOperations(operations));
+
+ setLegacyFieldsForInstanceOperation(operation);
+ }
+
+ /**
+ * Set the instance operation for this instance. This method also sets the
HELIX_ENABLED,
+ * HELIX_DISABLED_REASON, and HELIX_DISABLED_TYPE fields for backwards
compatibility.
+ *
+ * @param operation the instance operation
+ */
+ public void setInstanceOperation(InstanceOperation operation) {
+ // TODO: This can be removed when HELIX_ENABLED is removed.
+ // This is used for backwards compatibility. getInstanceOperation will
respect the old
+ // legacy field of HELIX_ENABLED which could have been set by older
versions of Helix.
+ // If the HELIX_ENABLED is set by an older version of Helix, we need to
sync that value to the
+ // new InstanceOperation field when any new instance operation is set.
+ // We only need to do this if there is already an instance operation set.
If there isn't, then
+ // the default is ENABLE with a source of DEFAULT.
+ if (!getInstanceOperations().isEmpty()) {
+ setInstanceOperationHelper(getInstanceOperation());
Review Comment:
Refactored to make it more efficient
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
