[GitHub] [helix] dasahcc commented on a change in pull request #1129: Validate instance topology configuration before let it comes online

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

dasahcc commented on a change in pull request #1129:
URL: https://github.com/apache/helix/pull/1129#discussion_r467272861



##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/PerInstanceAccessor.java
##########
@@ -308,20 +309,34 @@ public Response updateInstanceConfig(@PathParam("clusterId") String clusterId,
     }
     InstanceConfig instanceConfig = new InstanceConfig(record);
     ConfigAccessor configAccessor = getConfigAccessor();
+
     try {
+      /*
+       * Even if the instance is disabled, non-valid instance topology config will cause rebalance
+       * failure. We are doing the check whenever user updates InstanceConfig.
+       */
+      if (command == Command.delete || command == Command.update) {
+        validateDeltaTopologySettingInInstanceConfig(clusterId, instanceName, configAccessor,

Review comment:
       Why we dont call this in the switch case? It will make the code more clear. By doing that, you can let this validateDeltaTopologySettingInInstanceConfig, return a Config if it is valid. Otherwise, return null. 
   
   So the following code will based on the return result to do the write to ZK.

##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ParticipantManager.java
##########
@@ -187,26 +187,28 @@ private void joinCluster() {
       LOG.info("auto registration is false for cluster" + _clusterName);
     }
 
+    InstanceConfig instanceConfig;
     if (!ZKUtil.isInstanceSetup(_zkclient, _clusterName, _instanceName, _instanceType)) {
       if (!autoJoin) {
         throw new HelixException("Initial cluster structure is not set up for instance: "
             + _instanceName + ", instanceType: " + _instanceType);
+      }
+      if (!autoRegistration) {
+        LOG.info(_instanceName + " is auto-joining cluster: " + _clusterName);
+        instanceConfig = HelixUtil.composeInstanceConfig(_instanceName);
       } else {
-        if (!autoRegistration) {
-          LOG.info(_instanceName + " is auto-joining cluster: " + _clusterName);
-          _helixAdmin.addInstance(_clusterName, HelixUtil.composeInstanceConfig(_instanceName));
-        } else {
-          LOG.info(_instanceName + " is auto-registering cluster: " + _clusterName);
-          CloudInstanceInformation cloudInstanceInformation = getCloudInstanceInformation();
-          String domain = cloudInstanceInformation
-              .get(CloudInstanceInformation.CloudInstanceField.FAULT_DOMAIN.name()) + _instanceName;
-
-          InstanceConfig instanceConfig = HelixUtil.composeInstanceConfig(_instanceName);
-          instanceConfig.setDomain(domain);
-          _helixAdmin.addInstance(_clusterName, instanceConfig);
-        }
+        LOG.info(_instanceName + " is auto-registering cluster: " + _clusterName);
+        CloudInstanceInformation cloudInstanceInformation = getCloudInstanceInformation();
+        String domain = cloudInstanceInformation
+            .get(CloudInstanceInformation.CloudInstanceField.FAULT_DOMAIN.name()) + _instanceName;
+        instanceConfig = HelixUtil.composeInstanceConfig(_instanceName);
+        instanceConfig.setDomain(domain);
       }
+      _helixAdmin.addInstance(_clusterName, instanceConfig);
     }
+    _configAccessor.getInstanceConfig(_clusterName, _instanceName)
+        .validateTopologySettingInInstanceConfig(_configAccessor.getClusterConfig(_clusterName),

Review comment:
       Why we dont validate it before we write to ZK? 




----------------------------------------------------------------
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.

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