[GitHub] [helix] qqu0127 commented on a change in pull request #2006: Add type and reason for batch disable/enable

[GitBox <gi...@apache.org>]

qqu0127 commented on a change in pull request #2006:
URL: https://github.com/apache/helix/pull/2006#discussion_r840940478



##########
File path: helix-common/src/main/java/org/apache/helix/StringProcessUtil.java
##########
@@ -0,0 +1,38 @@
+package org.apache.helix;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+
+public class StringProcessUtil {

Review comment:
       Apache license?

##########
File path: helix-common/src/main/java/org/apache/helix/StringProcessUtil.java
##########
@@ -0,0 +1,38 @@
+package org.apache.helix;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+
+public class StringProcessUtil {

Review comment:
       Depending on the use case, StringProcess is too generic a name, and right now this class seems specific to parsing config. Shall we consider rename?

##########
File path: helix-common/src/main/java/org/apache/helix/StringProcessUtil.java
##########
@@ -0,0 +1,38 @@
+package org.apache.helix;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+
+public class StringProcessUtil {

Review comment:
       Also let's add unit test for it, thanks.

##########
File path: helix-core/src/test/java/org/apache/helix/mock/MockHelixAdmin.java
##########
@@ -295,16 +302,75 @@ public void enableInstance(String clusterName, String instanceName, boolean enab
     _baseDataAccessor.set(instanceConfigPath, instanceConfig.getRecord(), 0);
   }
 
-  @Override public void enableInstance(String clusterName, List<String> instances,
-      boolean enabled) {
+  @Override
+  public void enableInstance(String clusterName, List<String> instances, boolean enabled) {
+    enableInstance(clusterName, instances, enabled, null, null);
+  }
+
+  @Override
+  public void enableInstance(String clusterName, List<String> instances, boolean enabled,
+      InstanceConstants.InstanceDisabledType disabledType, String reason) {
+
+    String path = PropertyPathBuilder.clusterConfig(clusterName);
+
+    if (!_baseDataAccessor.exists(path, 0)) {
+      _baseDataAccessor.create(path, new ZNRecord(clusterName), 0);
+    }
+
+    _baseDataAccessor.update(path, new DataUpdater<ZNRecord>() {
+      @Override
+      public ZNRecord update(ZNRecord currentData) {

Review comment:
       Let's try to reduce code copy-paste

##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -1927,14 +1933,30 @@ public ZNRecord update(ZNRecord currentData) {
         if (clusterConfig.getDisabledInstances() != null) {
           disabledInstances.putAll(clusterConfig.getDisabledInstances());
         }
-
         if (enabled) {
           disabledInstances.keySet().removeAll(instances);
         } else {
           for (String disabledInstance : instances) {
-            if (!disabledInstances.containsKey(disabledInstance)) {
-              disabledInstances.put(disabledInstance, String.valueOf(System.currentTimeMillis()));
+            Map<String, String> disableInfo = new TreeMap<>();
+            disableInfo.put(ClusterConfig.ClusterConfigProperty.HELIX_ENABLED_TIMESTAMP.toString(),
+                String.valueOf(System.currentTimeMillis()));
+            if (disabledType != null) {
+              if (disabledType
+                  .equals(InstanceConstants.InstanceDisabledType.INSTANCE_NOT_DISABLED)) {
+                throw new HelixException(
+                    "Can not set INSTANCE_NOT_DISABLED as disabled type to an instance");
+              }
+              disableInfo.put(ClusterConfig.ClusterConfigProperty.HELIX_DISABLED_TYPE.toString(),
+                  disabledType.toString());
+            }
+            if (reason != null) {
+              disableInfo.put(ClusterConfig.ClusterConfigProperty.HELIX_DISABLED_REASON.toString(),
+                  reason);
             }
+            // We allow user to override disabledType and reason for an already disabled instance.
+            // TODO: update the history ZNode
+            disabledInstances
+                .put(disabledInstance, StringProcessUtil.concatenateMapIntoString(disableInfo));

Review comment:
       nit: this is a little hard to read, can we make these a separate method?

##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -1045,4 +1053,28 @@ public int hashCode() {
   public String getClusterName() {
     return _record.getId();
   }
+
+  public String getPlainInstanceHelixDisabledType(String instanceName) {
+    return StringProcessUtil.concatenateConfigParser(getDisabledInstances().get(instanceName))
+        .get(ClusterConfigProperty.HELIX_DISABLED_TYPE.toString());
+  }

Review comment:
       Open discussion, can this and below methods be put in the util class instead? I think this ClusterConfig class is getting bloated. 




-- 
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-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org