junkaixue commented on code in PR #2801:
URL: https://github.com/apache/helix/pull/2801#discussion_r1628113175
##########
helix-common/src/main/java/org/apache/helix/constants/InstanceConstants.java:
##########
@@ -24,15 +24,52 @@ public class InstanceConstants {
/**
* The set of InstanceOperations that are not allowed to be populated in the
RoutingTableProvider.
*/
- public static final Set<InstanceOperation> UNSERVABLE_INSTANCE_OPERATIONS =
+ public static final Set<InstanceOperation> UNROUTABLE_INSTANCE_OPERATIONS =
Set.of(InstanceOperation.SWAP_IN, InstanceOperation.UNKNOWN);
+ @Deprecated
public enum InstanceDisabledType {
CLOUD_EVENT,
USER_OPERATION,
DEFAULT_INSTANCE_DISABLE_TYPE
}
+ public enum InstanceOperationSource {
+ AUTOMATION, ADMIN, USER;
Review Comment:
Have integer property for its priority? Say ADMIN(0), USER(1) and
AUTOMATION(2)?
##########
helix-common/src/main/java/org/apache/helix/constants/InstanceConstants.java:
##########
@@ -24,15 +24,52 @@ public class InstanceConstants {
/**
* The set of InstanceOperations that are not allowed to be populated in the
RoutingTableProvider.
*/
- public static final Set<InstanceOperation> UNSERVABLE_INSTANCE_OPERATIONS =
+ public static final Set<InstanceOperation> UNROUTABLE_INSTANCE_OPERATIONS =
Set.of(InstanceOperation.SWAP_IN, InstanceOperation.UNKNOWN);
+ @Deprecated
public enum InstanceDisabledType {
CLOUD_EVENT,
USER_OPERATION,
DEFAULT_INSTANCE_DISABLE_TYPE
}
+ public enum InstanceOperationSource {
+ AUTOMATION, ADMIN, USER;
+
+ /**
+ * Convert from InstanceOperationTrigger to DisabledType
+ *
+ * @param trigger InstanceOperationTrigger
+ * @return InstanceDisabledType
+ */
+ public static InstanceDisabledType
instanceOperationSourceToInstanceDisabledType(
Review Comment:
Let's remove this type concept. It is redundant and could be confusing. If
we already defined the AUTOMATION. The source is AUTOMATION then it does not
have to be CLOUD_EVENT. User can categorize it with any types. Don't blindly
bundles these two together.
##########
helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java:
##########
@@ -328,75 +462,192 @@ public String getInstanceDisabledReason() {
*
* @return Return instance disabled type
(org.apache.helix.constants.InstanceConstants.InstanceDisabledType)
* Default is am empty string.
+ * @deprecated This method is deprecated. Please use
getInstanceOperation().getSource
+ *() instead.
*/
+ @Deprecated
public String getInstanceDisabledType() {
- if
(!getInstanceOperation().equals(InstanceConstants.InstanceOperation.DISABLE)) {
+ if (_record.getBooleanField(InstanceConfigProperty.HELIX_ENABLED.name(),
+ HELIX_ENABLED_DEFAULT_VALUE)) {
return InstanceConstants.INSTANCE_NOT_DISABLED;
}
return
_record.getStringField(InstanceConfigProperty.HELIX_DISABLED_TYPE.name(),
InstanceConstants.InstanceDisabledType.DEFAULT_INSTANCE_DISABLE_TYPE.name());
}
+ private List<InstanceOperation> getInstanceOperations() {
+ if (_deserializedInstanceOperations == null ||
_deserializedInstanceOperations.isEmpty()) {
+ // If the _deserializedInstanceOperationMap is not set, then we need to
build it from the real
+ // helix property HELIX_INSTANCE_OPERATION_MAP.
+ List<String> instanceOperations =
+
_record.getListField(InstanceConfigProperty.HELIX_INSTANCE_OPERATIONS.name());
+ List<InstanceOperation> newDeserializedInstanceOperations = new
ArrayList<>();
+ if (instanceOperations != null) {
+ for (String serializedInstanceOperation : instanceOperations) {
+ try {
+ Map<String, String> properties =
_objectMapper.readValue(serializedInstanceOperation,
+ new TypeReference<Map<String, String>>() {
+ });
+ newDeserializedInstanceOperations.add(new
InstanceOperation(properties));
+ } catch (JsonProcessingException e) {
+ _logger.error(
+ "Failed to deserialize instance operation for instance: " +
_record.getId(), e);
+ }
+ }
+ }
+ _deserializedInstanceOperations = newDeserializedInstanceOperations;
+ }
+
+ 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(InstanceConstants.InstanceOperation
operation) {
- _record.setSimpleField(InstanceConfigProperty.INSTANCE_OPERATION.name(),
- operation == null ? "" : operation.name());
- if (operation == null || operation ==
InstanceConstants.InstanceOperation.ENABLE
- || operation == InstanceConstants.InstanceOperation.DISABLE) {
+ public void setInstanceOperation(InstanceOperation operation) {
+ List<InstanceOperation> deserializedInstanceOperations =
getInstanceOperations();
+
+ if (operation.getSource() ==
InstanceConstants.InstanceOperationSource.ADMIN) {
+ deserializedInstanceOperations.clear();
+ } else {
+ // Remove the instance operation with the same trigger if it exists.
+ deserializedInstanceOperations.removeIf(
+ instanceOperation -> instanceOperation.getSource() ==
operation.getSource());
+ }
+ if (operation.getOperation() !=
InstanceConstants.InstanceOperation.ENABLE) {
+ deserializedInstanceOperations.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.
+ 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
// is on older version.
- // TODO: Remove this when we are sure that all users are using the new
field INSTANCE_OPERATION.
- setInstanceEnabledHelper(!(operation ==
InstanceConstants.InstanceOperation.DISABLE));
+
+ if (_record.getBooleanField(InstanceConfigProperty.HELIX_ENABLED.name(),
true)) {
+ // Check if it is already disabled, if yes, then we don't need to set
HELIX_ENABLED and HELIX_ENABLED_TIMESTAMP
+ setInstanceEnabledHelper(false, operation.getTimestamp());
+ }
+
+
_record.setSimpleField(InstanceConfigProperty.HELIX_DISABLED_REASON.name(),
+ operation.getReason());
+ _record.setSimpleField(InstanceConfigProperty.HELIX_DISABLED_TYPE.name(),
+
InstanceConstants.InstanceOperationSource.instanceOperationSourceToInstanceDisabledType(
+ operation.getSource()).name());
+ } else if (operation.getOperation() ==
InstanceConstants.InstanceOperation.ENABLE) {
Review Comment:
We need to think about the backward compatible behavior.
Say the instance was disabled in old field, but no new instance operations.
Then we should let it be disabled.
My suggestion for this implementation to get old field disable time and
merge in the new operation stack. Then we can populate it right and not have
these branches.
##########
helix-common/src/main/java/org/apache/helix/constants/InstanceConstants.java:
##########
@@ -24,15 +24,52 @@ public class InstanceConstants {
/**
* The set of InstanceOperations that are not allowed to be populated in the
RoutingTableProvider.
*/
- public static final Set<InstanceOperation> UNSERVABLE_INSTANCE_OPERATIONS =
+ public static final Set<InstanceOperation> UNROUTABLE_INSTANCE_OPERATIONS =
Set.of(InstanceOperation.SWAP_IN, InstanceOperation.UNKNOWN);
+ @Deprecated
public enum InstanceDisabledType {
CLOUD_EVENT,
USER_OPERATION,
DEFAULT_INSTANCE_DISABLE_TYPE
}
+ public enum InstanceOperationSource {
+ AUTOMATION, ADMIN, USER;
+
+ /**
+ * Convert from InstanceOperationTrigger to DisabledType
+ *
+ * @param trigger InstanceOperationTrigger
+ * @return InstanceDisabledType
+ */
+ public static InstanceDisabledType
instanceOperationSourceToInstanceDisabledType(
Review Comment:
If we would like to offer this categorization, let's just give an entry that
user can fill any type there.
This type translation I would assume the applications define in their code.
##########
helix-core/src/main/java/org/apache/helix/cloud/event/helix/DefaultCloudEventCallbackImpl.java:
##########
@@ -72,10 +80,14 @@ public void enableInstance(HelixManager manager, Object
eventInfo) {
HelixEventHandlingUtil
.updateCloudEventOperationInClusterConfig(manager.getClusterName(),
instanceName,
manager.getHelixDataAccessor().getBaseDataAccessor(), true,
message);
- if (HelixEventHandlingUtil.isInstanceDisabledForCloudEvent(instanceName,
accessor)) {
-
manager.getClusterManagmentTool().enableInstance(manager.getClusterName(),
instanceName, true,
- InstanceConstants.InstanceDisabledType.CLOUD_EVENT, message);
- }
+ InstanceUtil instanceUtil = new InstanceUtil(manager.getConfigAccessor(),
Review Comment:
Util usually are static?
##########
helix-core/src/main/java/org/apache/helix/util/InstanceUtil.java:
##########
@@ -0,0 +1,176 @@
+package org.apache.helix.util;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.List;
+import java.util.stream.Collectors;
+
+import com.google.common.collect.ImmutableSet;
+import org.apache.helix.AccessOption;
+import org.apache.helix.BaseDataAccessor;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.PropertyPathBuilder;
+import org.apache.helix.constants.InstanceConstants;
+import org.apache.helix.manager.zk.ZKHelixDataAccessor;
+import org.apache.helix.manager.zk.ZkBaseDataAccessor;
+import org.apache.helix.model.ClusterTopologyConfig;
+import org.apache.helix.model.HelixConfigScope;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.builder.HelixConfigScopeBuilder;
+import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+
+public class InstanceUtil {
+ private final BaseDataAccessor<ZNRecord> _baseDataAccessor;
+ private final ConfigAccessor _configAccessor;
+
+ public InstanceUtil(RealmAwareZkClient zkClient) {
+ _baseDataAccessor = new ZkBaseDataAccessor<>(zkClient);
+ _configAccessor = new ConfigAccessor(zkClient);
+ }
Review Comment:
As Util, it should be static. Have a client bundled here will be risky. If
user already close the client and use a new client, then this class should be
destroyed, not efficient.
This is why usually util is static methods and it can take zkclient as one
argument as needed.
##########
helix-core/src/main/java/org/apache/helix/util/InstanceUtil.java:
##########
@@ -0,0 +1,176 @@
+package org.apache.helix.util;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.List;
+import java.util.stream.Collectors;
+
+import com.google.common.collect.ImmutableSet;
+import org.apache.helix.AccessOption;
+import org.apache.helix.BaseDataAccessor;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.PropertyPathBuilder;
+import org.apache.helix.constants.InstanceConstants;
+import org.apache.helix.manager.zk.ZKHelixDataAccessor;
+import org.apache.helix.manager.zk.ZkBaseDataAccessor;
+import org.apache.helix.model.ClusterTopologyConfig;
+import org.apache.helix.model.HelixConfigScope;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.builder.HelixConfigScopeBuilder;
+import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+
+public class InstanceUtil {
+ private final BaseDataAccessor<ZNRecord> _baseDataAccessor;
+ private final ConfigAccessor _configAccessor;
+
+ public InstanceUtil(RealmAwareZkClient zkClient) {
+ _baseDataAccessor = new ZkBaseDataAccessor<>(zkClient);
+ _configAccessor = new ConfigAccessor(zkClient);
+ }
+
+ public InstanceUtil(ConfigAccessor configAccessor,
BaseDataAccessor<ZNRecord> baseAccessor) {
+ _baseDataAccessor = baseAccessor;
+ _configAccessor = configAccessor;
+ }
+
+ public static void validateInstanceOperationTransition(InstanceConfig
matchingLogicalIdInstance,
Review Comment:
Why not build a static map like:
ENABLE -> Set(DISABLE, EVACUTATE)
DISABLE. -> Set(....)
So the logic will just be
map.get(currentOperation).contains(futureOperation).
--
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
