[GitHub] [helix] xyuanlu commented on a change in pull request #1850: Use activate/DeactivateInstances as keyword in PartitionAssignment API

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

xyuanlu commented on a change in pull request #1850:
URL: https://github.com/apache/helix/pull/1850#discussion_r694515406



##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ResourceAssignmentOptimizerAccessor.java
##########
@@ -190,42 +184,44 @@ private InputFields readInput(String content)
   private ClusterState readClusterStateAndValidateInput(String clusterId, InputFields inputFields)
       throws InvalidParameterException {
 
+    // One instance can only exist in one of the list in InstanceChange.
+    // Validate the intersection is empty.
+    for (String instance : inputFields.activatedInstances) {
+      if (inputFields.deactivatedInstances.contains(instance)) {
+        LOG.error(
+            "Instance " + instance + " in both ActivateInstances and DeactivateInstances list.");
+      }
+    }
+
+    // Add instances to current liveInstances
     ClusterState clusterState = new ClusterState();
     ConfigAccessor cfgAccessor = getConfigAccessor();
     HelixDataAccessor dataAccessor = getDataAccssor(clusterId);
     clusterState.resources = dataAccessor.getChildNames(dataAccessor.keyBuilder().idealStates());
     // Add existing live instances and new instances from user input to instances list.
-    clusterState.instances = dataAccessor.getChildNames(dataAccessor.keyBuilder().liveInstances());
-    clusterState.instances.addAll(inputFields.newInstances);
-
-    // Check if to be removed instances and old instances in swap node exist in live instance.
-    if (!inputFields.nodeSwap.isEmpty() || !inputFields.instancesToRemove.isEmpty()) {
-      Set<String> liveInstanceSet = new HashSet<>(clusterState.instances);
-      for (Map.Entry<String, String> nodeSwapPair : inputFields.nodeSwap.entrySet()) {
-        if (!liveInstanceSet.contains(nodeSwapPair.getKey())) {
-          throw new InvalidParameterException("Invalid input: instance [" + nodeSwapPair.getKey()
-              + "] in SwapInstances does not exist in cluster.");
-        }
-      }
-      for (String instanceToRemove : inputFields.instancesToRemove) {
-        if (!liveInstanceSet.contains(instanceToRemove)) {
-          throw new InvalidParameterException("Invalid input: instance [" + instanceToRemove
-              + "] in RemoveInstances does not exist in cluster.");
-        }
-      }
-      if (!inputFields.instancesToRemove.isEmpty()) {
-        clusterState.instances.removeIf(inputFields.instancesToRemove::contains);
-      }
-    }
+    Set<String> liveInstancesSet =
+        new HashSet<>(dataAccessor.getChildNames(dataAccessor.keyBuilder().liveInstances()));
+    liveInstancesSet.addAll(
+        inputFields.activatedInstances.stream().filter(s -> !liveInstancesSet.contains(s))
+            .collect(Collectors.toList()));
 
     // Read instance and cluster config.
-    // It will throw exception if there is no instanceConfig for newly added instance.
-    for (String instance : clusterState.instances) {
+    // Throw exception if there is no instanceConfig for activatedInstances instance.
+    for (String instance : liveInstancesSet) {

Review comment:
       Please correct me if I am wrong. When the code reach here, the `liveInstancesSet` contains current live instances and the instances in `activatedInstances` list? 
   For instances in deactivated, these instances can only be in this list if they are currently under liveInstance, so they have corresponding config.  




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