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

Reply via email to