xyuanlu commented on code in PR #2987: URL: https://github.com/apache/helix/pull/2987#discussion_r1920888609
########## 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: Do you think we could move the write back logic inside `setInstanceOperationHelper `? Now if Helix_enabled is set, we call `setInstanceOperationHelper ` twice and in each call we also call `setLegacyFieldsForInstanceOperation`. I think in the first `setInstanceOperationHelper`, probably there is no need to overwrite helix_enabled again. -- 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: reviews-unsubscr...@helix.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org