diff --git a/helix-common/src/main/java/org/apache/helix/constants/InstanceConstants.java b/helix-common/src/main/java/org/apache/helix/constants/InstanceConstants.java index df6c37bb61..bc87a4f681 100644 --- a/helix-common/src/main/java/org/apache/helix/constants/InstanceConstants.java +++ b/helix-common/src/main/java/org/apache/helix/constants/InstanceConstants.java @@ -54,7 +54,9 @@ public int getPriority() { * * @param disabledType InstanceDisabledType * @return InstanceOperationTrigger + * @deprecated The concept of InstanceDisabledType mapping directly to an InstanceOperationSource is no longer used. */ + @Deprecated public static InstanceOperationSource instanceDisabledTypeToInstanceOperationSource( InstanceDisabledType disabledType) { switch (disabledType) { diff --git a/helix-core/src/main/java/org/apache/helix/cloud/event/helix/DefaultCloudEventCallbackImpl.java b/helix-core/src/main/java/org/apache/helix/cloud/event/helix/DefaultCloudEventCallbackImpl.java index 20c5001164..d41c1157a5 100644 --- a/helix-core/src/main/java/org/apache/helix/cloud/event/helix/DefaultCloudEventCallbackImpl.java +++ b/helix-core/src/main/java/org/apache/helix/cloud/event/helix/DefaultCloudEventCallbackImpl.java @@ -51,14 +51,9 @@ public void disableInstance(HelixManager manager, Object eventInfo) { LOG.info("DefaultCloudEventCallbackImpl disable Instance {}", manager.getInstanceName()); if (InstanceValidationUtil .isEnabled(manager.getHelixDataAccessor(), manager.getInstanceName())) { - InstanceUtil.setInstanceOperation(manager.getConfigAccessor(), - manager.getHelixDataAccessor().getBaseDataAccessor(), manager.getClusterName(), - manager.getInstanceName(), - new InstanceConfig.InstanceOperation.Builder().setOperation( - InstanceConstants.InstanceOperation.DISABLE) - .setSource(InstanceConstants.InstanceOperationSource.AUTOMATION) - .setReason(message) - .build()); + manager.getClusterManagmentTool() + .enableInstance(manager.getClusterName(), manager.getInstanceName(), false, + InstanceConstants.InstanceDisabledType.CLOUD_EVENT, message); } HelixEventHandlingUtil.updateCloudEventOperationInClusterConfig(manager.getClusterName(), manager.getInstanceName(), manager.getHelixDataAccessor().getBaseDataAccessor(), false, @@ -79,13 +74,10 @@ public void enableInstance(HelixManager manager, Object eventInfo) { HelixEventHandlingUtil .updateCloudEventOperationInClusterConfig(manager.getClusterName(), instanceName, manager.getHelixDataAccessor().getBaseDataAccessor(), true, message); - InstanceUtil.setInstanceOperation(manager.getConfigAccessor(), - manager.getHelixDataAccessor().getBaseDataAccessor(), manager.getClusterName(), - manager.getInstanceName(), - new InstanceConfig.InstanceOperation.Builder().setOperation( - InstanceConstants.InstanceOperation.ENABLE) - .setSource(InstanceConstants.InstanceOperationSource.AUTOMATION).setReason(message) - .build()); + if (HelixEventHandlingUtil.isInstanceDisabledForCloudEvent(instanceName, accessor)) { + manager.getClusterManagmentTool().enableInstance(manager.getClusterName(), instanceName, true, + InstanceConstants.InstanceDisabledType.CLOUD_EVENT, message); + } } /** diff --git a/helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java b/helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java index b091b70ca8..d5998a166f 100644 --- a/helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java +++ b/helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java @@ -747,7 +747,7 @@ public boolean forceKillInstance(String clusterName, String instanceName, String InstanceConfig.InstanceOperation instanceOperationObj = new InstanceConfig.InstanceOperation.Builder() .setOperation(InstanceConstants.InstanceOperation.UNKNOWN).setReason(reason) - .setSource(operationSource != null ? operationSource : InstanceConstants.InstanceOperationSource.USER).build(); + .setSource(operationSource).build(); InstanceConfig instanceConfig = getInstanceConfig(clusterName, instanceName); instanceConfig.setInstanceOperation(instanceOperationObj); @@ -2363,12 +2363,17 @@ public ZNRecord update(ZNRecord currentData) { } InstanceConfig config = new InstanceConfig(currentData); - config.setInstanceOperation(new InstanceConfig.InstanceOperation.Builder().setOperation( - enabled ? InstanceConstants.InstanceOperation.ENABLE - : InstanceConstants.InstanceOperation.DISABLE).setReason(reason).setSource( - disabledType != null - ? InstanceConstants.InstanceOperationSource.instanceDisabledTypeToInstanceOperationSource( - disabledType) : null).build()); + config.setInstanceEnabled(enabled); + if (!enabled) { + // new disabled type and reason will overwrite existing ones. + config.resetInstanceDisabledTypeAndReason(); + if (reason != null) { + config.setInstanceDisabledReason(reason); + } + if (disabledType != null) { + config.setInstanceDisabledType(disabledType); + } + } return config.getRecord(); } }, AccessOption.PERSISTENT); diff --git a/helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java b/helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java index 7b5faddc34..b53e0b1fb3 100644 --- a/helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java +++ b/helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java @@ -22,12 +22,14 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.Comparator; import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; import java.util.stream.Collectors; +import java.util.stream.IntStream; import javax.annotation.Nullable; @@ -49,6 +51,8 @@ * Instance configurations */ public class InstanceConfig extends HelixProperty { + private static final Logger logger = LoggerFactory.getLogger(InstanceConfig.class.getName()); + /** * Configurable characteristics of an instance */ @@ -67,14 +71,22 @@ public enum InstanceConfigProperty { DELAY_REBALANCE_ENABLED, MAX_CONCURRENT_TASK, INSTANCE_INFO_MAP, - INSTANCE_CAPACITY_MAP, TARGET_TASK_THREAD_POOL_SIZE, HELIX_INSTANCE_OPERATIONS + INSTANCE_CAPACITY_MAP, + TARGET_TASK_THREAD_POOL_SIZE, + HELIX_INSTANCE_OPERATIONS } public static class InstanceOperation { + private static final String DEFAULT_INSTANCE_OPERATION_SOURCE = + InstanceConstants.InstanceOperationSource.USER.name(); private final Map _properties; private enum InstanceOperationProperties { - OPERATION, REASON, SOURCE, TIMESTAMP + OPERATION, + REASON, + SOURCE, + TIMESTAMP, + LEGACY_DISABLED_TYPE } private InstanceOperation(@Nullable Map properties) { @@ -91,6 +103,7 @@ public static class Builder { /** * Set the operation type for this instance operation. + * * @param operationType InstanceOperation type of this instance operation. */ public Builder setOperation(@Nullable InstanceConstants.InstanceOperation operationType) { @@ -102,22 +115,44 @@ public Builder setOperation(@Nullable InstanceConstants.InstanceOperation operat /** * Set the reason for this instance operation. - * @param reason + * + * @param reason the reason for this instance operation. */ public Builder setReason(String reason) { - _properties.put(InstanceOperationProperties.REASON.name(), reason != null ? reason : ""); + if (reason == null) { + logger.error("Reason cannot be set to null. Skipped setting the field."); + return this; + } + _properties.put(InstanceOperationProperties.REASON.name(), reason); return this; } /** * Set the source for this instance operation. - * @param source InstanceOperationSource - * that caused this instance operation to be triggered. + * + * @param source InstanceOperationSource that caused this instance operation to be triggered. + * @return Builder */ public Builder setSource(InstanceConstants.InstanceOperationSource source) { _properties.put(InstanceOperationProperties.SOURCE.name(), - source == null ? InstanceConstants.InstanceOperationSource.USER.name() - : source.name()); + source == null ? DEFAULT_INSTANCE_OPERATION_SOURCE : source.name()); + return this; + } + + /** + * Set the HELIX_DISABLED_TYPE which is a legacy field that must be stored, so we can write it + * back when we write back to the legacy fields. + * + * @param disabledType InstanceDisabledType + * @return Builder + */ + private Builder setLegacyDisabledType(InstanceConstants.InstanceDisabledType disabledType) { + if (disabledType == null) { + logger.error("LEGACY_DISABLED_TYPE cannot be set to null. Skipped setting the field."); + return this; + } + _properties.put(InstanceOperationProperties.LEGACY_DISABLED_TYPE.name(), + disabledType.name()); return this; } @@ -126,6 +161,8 @@ public InstanceOperation build() throws IllegalArgumentException { throw new IllegalArgumentException( "Instance operation type is not set, this is a required field."); } + _properties.putIfAbsent(InstanceOperationProperties.SOURCE.name(), + DEFAULT_INSTANCE_OPERATION_SOURCE); _properties.put(InstanceOperationProperties.TIMESTAMP.name(), String.valueOf(System.currentTimeMillis())); return new InstanceOperation(_properties); @@ -134,6 +171,7 @@ public InstanceOperation build() throws IllegalArgumentException { /** * Get the operation type of this instance operation. + * * @return the InstanceOperation type */ public InstanceConstants.InstanceOperation getOperation() throws IllegalArgumentException { @@ -142,8 +180,8 @@ public InstanceConstants.InstanceOperation getOperation() throws IllegalArgument } /** - * Get the reason for this instance operation. - * If the reason is not set, it will default to an empty string. + * Get the reason for this instance operation. If the reason is not set, it will default to an + * empty string. * * @return the reason for this instance operation. */ @@ -152,12 +190,10 @@ public String getReason() { } /** - * Get the InstanceOperationSource - * that caused this instance operation to be triggered. - * If the source is not set, it will default to DEFAULT. + * Get the InstanceOperationSource that caused this instance operation to be triggered. If the + * source is not set, it will default to DEFAULT. * - * @return the InstanceOperationSource - *that caused this instance operation to be triggered. + * @return the InstanceOperationSource that caused this instance operation to be triggered. */ public InstanceConstants.InstanceOperationSource getSource() { return InstanceConstants.InstanceOperationSource.valueOf( @@ -165,6 +201,12 @@ public InstanceConstants.InstanceOperationSource getSource() { InstanceConstants.InstanceOperationSource.USER.name())); } + private InstanceConstants.InstanceDisabledType getLegacyDisabledType() { + return InstanceConstants.InstanceDisabledType.valueOf( + _properties.getOrDefault(InstanceOperationProperties.LEGACY_DISABLED_TYPE.name(), + InstanceConstants.InstanceDisabledType.DEFAULT_INSTANCE_DISABLE_TYPE.name())); + } + /** * Get the timestamp (milliseconds from epoch) when this instance operation was triggered. * @@ -442,7 +484,8 @@ public void setInstanceDisabledReason(String disabledReason) { */ @Deprecated public void setInstanceDisabledType(InstanceConstants.InstanceDisabledType disabledType) { - if (getInstanceOperation().getOperation().equals(InstanceConstants.InstanceOperation.DISABLE)) { + if (getInstanceOperation().getOperation().equals(InstanceConstants.InstanceOperation.DISABLE) + && disabledType != InstanceConstants.InstanceDisabledType.DEFAULT_INSTANCE_DISABLE_TYPE) { _record.setSimpleField(InstanceConfigProperty.HELIX_DISABLED_TYPE.name(), disabledType.name()); } @@ -501,50 +544,42 @@ private List getInstanceOperations() { return _deserializedInstanceOperations; } - /** - * 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) { - List deserializedInstanceOperations = getInstanceOperations(); - + private void updateInstanceOperation(List operations, + InstanceOperation operation) { if (operation.getSource() == InstanceConstants.InstanceOperationSource.ADMIN) { - deserializedInstanceOperations.clear(); + operations.clear(); } else { - // Remove the instance operation with the same source if it exists. - deserializedInstanceOperations.removeIf( - instanceOperation -> instanceOperation.getSource() == operation.getSource()); + // Remove existing operation with the same source. + operations.removeIf(op -> op.getSource() == operation.getSource()); } + if (operation.getOperation() == InstanceConstants.InstanceOperation.ENABLE) { - // Insert the operation after the last ENABLE or at the beginning if there isn't ENABLE in the list. - int insertIndex = 0; - for (int i = deserializedInstanceOperations.size() - 1; i >= 0; i--) { - if (deserializedInstanceOperations.get(i).getOperation() - == InstanceConstants.InstanceOperation.ENABLE) { - insertIndex = i + 1; - break; - } - } - deserializedInstanceOperations.add(insertIndex, operation); + // Insert ENABLE operation after the last existing ENABLE, or at the beginning. + int insertIndex = (int) IntStream.range(0, operations.size()).filter( + i -> operations.get(i).getOperation() == InstanceConstants.InstanceOperation.ENABLE) + .reduce((first, second) -> second).orElse(-1) + 1; + + operations.add(insertIndex, operation); } else { - deserializedInstanceOperations.add(operation); + operations.add(operation); } - // Set the actual field in the ZnRecord - _record.setListField(InstanceConfigProperty.HELIX_INSTANCE_OPERATIONS.name(), - deserializedInstanceOperations.stream().map(instanceOperation -> { - try { - return _objectMapper.writeValueAsString(instanceOperation.getProperties()); - } catch (JsonProcessingException e) { - throw new HelixException( - "Failed to serialize instance operation for instance: " + _record.getId() - + " Can't set the instance operation to: " + operation.getOperation(), e); - } - }).collect(Collectors.toList())); + } - // TODO: Remove this when we are sure that all users are using the new InstanceOperation only and HELIX_ENABLED is removed. + private List serializeInstanceOperations(List operations) { + return operations.stream().map(op -> { + try { + return _objectMapper.writeValueAsString(op.getProperties()); + } catch (JsonProcessingException e) { + throw new HelixException( + "Failed to serialize instance operation for instance: " + _record.getId() + + ". Can't set the instance operation to: " + op.getOperation(), e); + } + }).collect(Collectors.toList()); + } + + private void setLegacyFieldsForInstanceOperation(InstanceOperation operation) { + // TODO: Remove this when we are sure that all users are using the new InstanceOperation only + // and HELIX_ENABLED is removed. if (operation.getOperation() == InstanceConstants.InstanceOperation.DISABLE) { // We are still setting the HELIX_ENABLED field for backwards compatibility. // It is possible that users will be using earlier version of HelixAdmin or helix-rest @@ -555,31 +590,57 @@ 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()); - } + /** + * 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) { + List operations = getInstanceOperations(); + + // 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 (!operations.isEmpty()) { + updateInstanceOperation(operations, getInstanceOperation()); } + + // Set the instance operation passed in. + updateInstanceOperation(operations, operation); + + // Serialize and update ZnRecord. + _record.setListField(InstanceConfigProperty.HELIX_INSTANCE_OPERATIONS.name(), + serializeInstanceOperations(operations)); + + setLegacyFieldsForInstanceOperation(operation); } /** @@ -648,12 +709,19 @@ public InstanceOperation getInstanceOperation() { HELIX_ENABLED_DEFAULT_VALUE) && (InstanceConstants.INSTANCE_DISABLED_OVERRIDABLE_OPERATIONS.contains( activeInstanceOperation.getOperation()))) { - return new InstanceOperation.Builder().setOperation( - InstanceConstants.InstanceOperation.DISABLE).setReason(getInstanceDisabledReason()) - .setSource( - InstanceConstants.InstanceOperationSource.instanceDisabledTypeToInstanceOperationSource( - InstanceConstants.InstanceDisabledType.valueOf(getInstanceDisabledType()))) - .build(); + InstanceOperation.Builder instanceOperationBuilder = + new InstanceOperation.Builder().setOperation(InstanceConstants.InstanceOperation.DISABLE) + .setReason(getInstanceDisabledReason()); + + try { + instanceOperationBuilder.setLegacyDisabledType( + InstanceConstants.InstanceDisabledType.valueOf(getInstanceDisabledType())); + } catch (IllegalArgumentException e) { + _logger.error("Invalid instance disabled type for instance: " + _record.getId() + + ". Defaulting to DEFAULT_INSTANCE_DISABLE_TYPE."); + } + + return instanceOperationBuilder.build(); } // if instance operation is DISABLE, we override it to ENABLE if HELIX_ENABLED set to true @@ -662,8 +730,10 @@ public InstanceOperation getInstanceOperation() { // we always set HELIX_ENABLED to false // 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(); + if (_record.getBooleanField(InstanceConfigProperty.HELIX_ENABLED.name(), + HELIX_ENABLED_DEFAULT_VALUE)) { + return new InstanceOperation.Builder().setOperation( + InstanceConstants.InstanceOperation.ENABLE).build(); } } @@ -1098,7 +1168,6 @@ public static InstanceConfig toInstanceConfig(String instanceId) { if (host != null && port > 0) { config.setHostName(host); config.setPort(String.valueOf(port)); - } if (config.getHostName() == null) { diff --git a/helix-core/src/test/java/org/apache/helix/cloud/event/TestDefaultCloudEventCallbackImpl.java b/helix-core/src/test/java/org/apache/helix/cloud/event/TestDefaultCloudEventCallbackImpl.java index 5ea42dc3e7..2f03cb3dd8 100644 --- a/helix-core/src/test/java/org/apache/helix/cloud/event/TestDefaultCloudEventCallbackImpl.java +++ b/helix-core/src/test/java/org/apache/helix/cloud/event/TestDefaultCloudEventCallbackImpl.java @@ -53,7 +53,7 @@ public void testDisableInstance() { .isEnabled(_manager.getHelixDataAccessor(), _instanceManager.getInstanceName())); Assert.assertEquals(_manager.getConfigAccessor() .getInstanceConfig(CLUSTER_NAME, _instanceManager.getInstanceName()).getInstanceOperation() - .getSource(), InstanceConstants.InstanceOperationSource.AUTOMATION); + .getSource(), InstanceConstants.InstanceOperationSource.USER); _admin.enableInstance(CLUSTER_NAME, _instanceManager.getInstanceName(), false); _impl.disableInstance(_instanceManager, null); diff --git a/helix-core/src/test/java/org/apache/helix/mock/MockHelixAdmin.java b/helix-core/src/test/java/org/apache/helix/mock/MockHelixAdmin.java index c5c5626ff6..5a1a8a5bcb 100644 --- a/helix-core/src/test/java/org/apache/helix/mock/MockHelixAdmin.java +++ b/helix-core/src/test/java/org/apache/helix/mock/MockHelixAdmin.java @@ -287,12 +287,16 @@ public void enableInstance(String clusterName, String instanceName, boolean enab ZNRecord record = (ZNRecord) _baseDataAccessor.get(instanceConfigPath, null, 0); InstanceConfig instanceConfig = new InstanceConfig(record); - instanceConfig.setInstanceOperation(new InstanceConfig.InstanceOperation.Builder().setOperation( - enabled ? InstanceConstants.InstanceOperation.ENABLE - : InstanceConstants.InstanceOperation.DISABLE).setReason(reason).build()); + instanceConfig.setInstanceEnabled(enabled); if (!enabled) { // TODO: Replace this when the HELIX_ENABLED and HELIX_DISABLED fields are removed. instanceConfig.resetInstanceDisabledTypeAndReason(); + if (reason != null) { + instanceConfig.setInstanceDisabledReason(reason); + } + if (disabledType != null) { + instanceConfig.setInstanceDisabledType(disabledType); + } } _baseDataAccessor.set(instanceConfigPath, instanceConfig.getRecord(), 0); } diff --git a/helix-core/src/test/java/org/apache/helix/model/TestInstanceConfig.java b/helix-core/src/test/java/org/apache/helix/model/TestInstanceConfig.java index f0081bec00..506710921e 100644 --- a/helix-core/src/test/java/org/apache/helix/model/TestInstanceConfig.java +++ b/helix-core/src/test/java/org/apache/helix/model/TestInstanceConfig.java @@ -56,7 +56,7 @@ public void testSetInstanceEnableWithReason() { instanceConfig.setInstanceDisabledType(InstanceConstants.InstanceDisabledType.USER_OPERATION); Assert.assertEquals(instanceConfig.getRecord().getSimpleFields() - .get(InstanceConfig.InstanceConfigProperty.HELIX_ENABLED.toString()), "true"); + .get(InstanceConfig.InstanceConfigProperty.HELIX_ENABLED.toString()), null); Assert.assertEquals(instanceConfig.getRecord().getSimpleFields() .get(InstanceConfig.InstanceConfigProperty.HELIX_DISABLED_REASON.toString()), null); Assert.assertEquals(instanceConfig.getRecord().getSimpleFields() @@ -81,14 +81,114 @@ public void testEnabledInstanceBackwardCompatibility() { InstanceConfig instanceConfig = new InstanceConfig.Builder().setInstanceOperation(InstanceConstants.InstanceOperation.DISABLE).build("id"); Assert.assertFalse(instanceConfig.getInstanceEnabled()); - // assume an old version client enabled the instance by setting HELIX_ENABLED to true + Assert.assertEquals(instanceConfig.getInstanceOperation().getOperation(), + InstanceConstants.InstanceOperation.DISABLE); + Assert.assertEquals(instanceConfig.getInstanceOperation().getSource(), + InstanceConstants.InstanceOperationSource.USER); + + // Assume an old version client enabled the instance by setting HELIX_ENABLED to true instanceConfig.getRecord().setSimpleField(InstanceConfig.InstanceConfigProperty.HELIX_ENABLED.name(), "true"); Assert.assertTrue(instanceConfig.getInstanceEnabled()); - instanceConfig.setInstanceOperation(InstanceConstants.InstanceOperation.ENABLE); + Assert.assertEquals(instanceConfig.getInstanceOperation().getOperation(), + InstanceConstants.InstanceOperation.ENABLE); + Assert.assertEquals(instanceConfig.getInstanceOperation().getSource(), + InstanceConstants.InstanceOperationSource.USER); - // disable the instance by setting HELIX_ENABLED to false + // Automation source then disables the instance and writes the HELIX_ENABLED field to USER source's + // instance operation + instanceConfig.setInstanceOperation(new InstanceConfig.InstanceOperation.Builder().setSource( + InstanceConstants.InstanceOperationSource.AUTOMATION) + .setOperation(InstanceConstants.InstanceOperation.DISABLE).setReason("disableReason") + .build()); + Assert.assertFalse(instanceConfig.getInstanceEnabled()); + Assert.assertEquals(instanceConfig.getInstanceOperation().getOperation(), + InstanceConstants.InstanceOperation.DISABLE); + Assert.assertEquals(instanceConfig.getInstanceOperation().getSource(), + InstanceConstants.InstanceOperationSource.AUTOMATION); + Assert.assertEquals(instanceConfig.getInstanceOperation().getReason(), "disableReason"); + Assert.assertEquals(instanceConfig.getInstanceDisabledType(), + InstanceConstants.InstanceDisabledType.DEFAULT_INSTANCE_DISABLE_TYPE.toString()); + Assert.assertEquals(instanceConfig.getInstanceDisabledReason(), "disableReason"); + + // Automation source then enables the instance + instanceConfig.setInstanceOperation(new InstanceConfig.InstanceOperation.Builder().setSource( + InstanceConstants.InstanceOperationSource.AUTOMATION) + .setOperation(InstanceConstants.InstanceOperation.ENABLE).build()); + // This should be enabled because we write the currently set legacy HELIX_ENABLED field into the instance operation + // associated with its source before the AUTOMATION source's operation is set. + Assert.assertTrue(instanceConfig.getInstanceEnabled()); + Assert.assertEquals(instanceConfig.getInstanceOperation().getOperation(), + InstanceConstants.InstanceOperation.ENABLE); + Assert.assertEquals(instanceConfig.getInstanceOperation().getSource(), + InstanceConstants.InstanceOperationSource.AUTOMATION); + + // Disable the instance with legacy HELIX_ENABLED field set to false instanceConfig.getRecord().setSimpleField(InstanceConfig.InstanceConfigProperty.HELIX_ENABLED.name(), "false"); Assert.assertFalse(instanceConfig.getInstanceEnabled()); + Assert.assertEquals(instanceConfig.getInstanceOperation().getOperation(), + InstanceConstants.InstanceOperation.DISABLE); + Assert.assertEquals(instanceConfig.getInstanceOperation().getSource(), + InstanceConstants.InstanceOperationSource.USER); + + // Enable the instance with automation source which should not override the HELIX_ENABLED field + // because the source that set HELIX_ENABLED is USER + instanceConfig.setInstanceOperation(new InstanceConfig.InstanceOperation.Builder().setSource( + InstanceConstants.InstanceOperationSource.AUTOMATION) + .setOperation(InstanceConstants.InstanceOperation.ENABLE).build()); + Assert.assertFalse(instanceConfig.getInstanceEnabled()); + Assert.assertEquals(instanceConfig.getInstanceOperation().getOperation(), + InstanceConstants.InstanceOperation.DISABLE); + Assert.assertEquals(instanceConfig.getInstanceOperation().getSource(), + InstanceConstants.InstanceOperationSource.USER); + + // Enable the instance with user source which should override the HELIX_ENABLED + instanceConfig.setInstanceOperation(new InstanceConfig.InstanceOperation.Builder().setSource( + InstanceConstants.InstanceOperationSource.USER) + .setOperation(InstanceConstants.InstanceOperation.ENABLE).build()); + Assert.assertTrue(instanceConfig.getInstanceEnabled()); + Assert.assertEquals(instanceConfig.getInstanceOperation().getOperation(), + InstanceConstants.InstanceOperation.ENABLE); + Assert.assertEquals(instanceConfig.getInstanceOperation().getSource(), + InstanceConstants.InstanceOperationSource.USER); + + // Disable the instance with legacy HELIX_ENABLED field set to false + instanceConfig.getRecord() + .setSimpleField(InstanceConfig.InstanceConfigProperty.HELIX_ENABLED.name(), "false"); + instanceConfig.setInstanceDisabledType(InstanceConstants.InstanceDisabledType.USER_OPERATION); + instanceConfig.setInstanceDisabledReason("foo"); + Assert.assertFalse(instanceConfig.getInstanceEnabled()); + Assert.assertEquals(instanceConfig.getInstanceOperation().getOperation(), + InstanceConstants.InstanceOperation.DISABLE); + Assert.assertEquals(instanceConfig.getInstanceOperation().getSource(), + InstanceConstants.InstanceOperationSource.USER); + + // Disable with automation source + instanceConfig.setInstanceOperation(new InstanceConfig.InstanceOperation.Builder().setSource( + InstanceConstants.InstanceOperationSource.AUTOMATION) + .setOperation(InstanceConstants.InstanceOperation.DISABLE).setReason("bar").build()); + Assert.assertFalse(instanceConfig.getInstanceEnabled()); + Assert.assertEquals(instanceConfig.getInstanceOperation().getOperation(), + InstanceConstants.InstanceOperation.DISABLE); + Assert.assertEquals(instanceConfig.getInstanceOperation().getSource(), + InstanceConstants.InstanceOperationSource.AUTOMATION); + Assert.assertEquals(instanceConfig.getInstanceOperation().getReason(), "bar"); + Assert.assertEquals(instanceConfig.getInstanceDisabledType(), + InstanceConstants.InstanceDisabledType.DEFAULT_INSTANCE_DISABLE_TYPE.name()); + Assert.assertEquals(instanceConfig.getInstanceDisabledReason(), "bar"); + + // Enable with automation source and return the instance to DISABLE with user source + instanceConfig.setInstanceOperation(new InstanceConfig.InstanceOperation.Builder().setSource( + InstanceConstants.InstanceOperationSource.AUTOMATION) + .setOperation(InstanceConstants.InstanceOperation.ENABLE).build()); + Assert.assertFalse(instanceConfig.getInstanceEnabled()); + Assert.assertEquals(instanceConfig.getInstanceOperation().getOperation(), + InstanceConstants.InstanceOperation.DISABLE); + Assert.assertEquals(instanceConfig.getInstanceOperation().getSource(), + InstanceConstants.InstanceOperationSource.USER); + Assert.assertEquals(instanceConfig.getInstanceOperation().getReason(), "foo"); + Assert.assertEquals(instanceConfig.getInstanceDisabledType(), + InstanceConstants.InstanceDisabledType.USER_OPERATION.name()); + Assert.assertEquals(instanceConfig.getInstanceDisabledReason(), "foo"); } @Test