This is an automated email from the ASF dual-hosted git repository.
xyuanlu pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/helix.git
The following commit(s) were added to refs/heads/master by this push:
new 0f95ef313 Support preserving history of HELIX_ENABLED legacy fields
when combo of instance operation and old helix version are used (#2987)
0f95ef313 is described below
commit 0f95ef3134d92ac46e6c75750f9284c0e1cece59
Author: Zachary Pinto <[email protected]>
AuthorDate: Fri Jan 17 20:52:02 2025 -0800
Support preserving history of HELIX_ENABLED legacy fields when combo of
instance operation and old helix version are used (#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.
---
.../apache/helix/constants/InstanceConstants.java | 2 +
.../event/helix/DefaultCloudEventCallbackImpl.java | 22 +-
.../org/apache/helix/manager/zk/ZKHelixAdmin.java | 19 +-
.../org/apache/helix/model/InstanceConfig.java | 233 +++++++++++++--------
.../event/TestDefaultCloudEventCallbackImpl.java | 2 +-
.../java/org/apache/helix/mock/MockHelixAdmin.java | 10 +-
.../org/apache/helix/model/TestInstanceConfig.java | 108 +++++++++-
7 files changed, 284 insertions(+), 112 deletions(-)
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 df6c37bb6..bc87a4f68 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 class InstanceConstants {
*
* @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 20c500116..d41c1157a 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 class DefaultCloudEventCallbackImpl {
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 class DefaultCloudEventCallbackImpl {
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 b091b70ca..d5998a166 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 class ZKHelixAdmin implements HelixAdmin {
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 class ZKHelixAdmin implements HelixAdmin {
}
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 7b5faddc3..b53e0b1fb 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 @@ package org.apache.helix.model;
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 @@ import org.slf4j.LoggerFactory;
* 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 class InstanceConfig extends HelixProperty {
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<String, String> _properties;
private enum InstanceOperationProperties {
- OPERATION, REASON, SOURCE, TIMESTAMP
+ OPERATION,
+ REASON,
+ SOURCE,
+ TIMESTAMP,
+ LEGACY_DISABLED_TYPE
}
private InstanceOperation(@Nullable Map<String, String> properties) {
@@ -91,6 +103,7 @@ public class InstanceConfig extends HelixProperty {
/**
* 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 class InstanceConfig extends HelixProperty {
/**
* 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 class InstanceConfig extends HelixProperty {
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 class InstanceConfig extends HelixProperty {
/**
* Get the operation type of this instance operation.
+ *
* @return the InstanceOperation type
*/
public InstanceConstants.InstanceOperation getOperation() throws
IllegalArgumentException {
@@ -142,8 +180,8 @@ public class InstanceConfig extends HelixProperty {
}
/**
- * 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 class InstanceConfig extends HelixProperty {
}
/**
- * 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 class InstanceConfig extends HelixProperty {
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 class InstanceConfig extends HelixProperty {
*/
@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 @@ public class InstanceConfig extends HelixProperty {
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<InstanceOperation> deserializedInstanceOperations =
getInstanceOperations();
-
+ private void updateInstanceOperation(List<InstanceOperation> 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<String> serializeInstanceOperations(List<InstanceOperation>
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 class InstanceConfig extends HelixProperty {
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<InstanceOperation> 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 class InstanceConfig extends HelixProperty {
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 class InstanceConfig extends HelixProperty {
// 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 class InstanceConfig extends HelixProperty {
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 5ea42dc3e..2f03cb3dd 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 class TestDefaultCloudEventCallbackImpl extends
ZkStandAloneCMTestBase {
.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 c5c5626ff..5a1a8a5bc 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 class MockHelixAdmin implements HelixAdmin {
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 f0081bec0..506710921 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 class TestInstanceConfig {
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 class TestInstanceConfig {
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
