xyuanlu commented on code in PR #2961:
URL: https://github.com/apache/helix/pull/2961#discussion_r1868471190


##########
helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/StoppableInstancesSelector.java:
##########
@@ -258,18 +293,21 @@ private Map<String, Set<String>> 
getOrderedZoneToInstancesMap(
   }
 
   /**
-   * Collect instances marked for evacuation in the current topology and add 
them into the given set
+   * Collect instances marked for evacuation, swap, or unkown in the current 
topology and add

Review Comment:
   nit: I would suggest refine on comment here. I think we are referring to all 
the instances in the cluster, not necessarily the topology aware cluster's 
topology tree. 



##########
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/InstancesAccessor.java:
##########
@@ -285,6 +296,28 @@ private Response batchGetStoppableInstances(String 
clusterId, JsonNode node, boo
         }
       }
 
+      ClusterTopology clusterTopology = 
clusterService.getClusterTopology(clusterId);
+      if (selectionBase != InstanceHealthSelectionBase.non_zone_based) {
+        if (!clusterService.isClusterTopologyAware(clusterId)) {
+          String message = "Cluster " + clusterId
+              + " is not topology aware. Please enable the topology in cluster 
config or set "
+              + "'selection_base' to 'non_zone_based'.";
+          _logger.error(message);
+          return badRequest(message);
+        }
+
+        // Find instances that lack topology information
+        Set<String> topologyUnawareInstances =

Review Comment:
   nit: we can merge the helper function here. 



##########
helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/StoppableInstancesSelector.java:
##########
@@ -136,6 +136,40 @@ public ObjectNode 
getStoppableInstancesCrossZones(List<String> instances,
     return result;
   }
 
+  /**
+   * Evaluates and collects stoppable instances not based on the zone order.
+   * The method iterates through instances, performing stoppable checks, and 
records reasons for
+   * non-stoppability.
+   *
+   * @param instances A list of instance to be evaluated.
+   * @param toBeStoppedInstances A list of instances presumed to be already 
stopped
+   * @return An ObjectNode containing:
+   *         - 'stoppableNode': List of instances that can be stopped.
+   *         - 'instance_not_stoppable_with_reasons': A map with the instance 
name as the key and
+   *         a list of reasons for non-stoppability as the value.
+   * @throws IOException
+   */
+  public ObjectNode getStoppableInstancesNonZoneBased(List<String> instances,
+      List<String> toBeStoppedInstances) throws IOException {
+    ObjectNode result = JsonNodeFactory.instance.objectNode();
+    ArrayNode stoppableInstances =
+        
result.putArray(InstancesAccessor.InstancesProperties.instance_stoppable_parallel.name());
+    ObjectNode failedStoppableInstances = result.putObject(
+        
InstancesAccessor.InstancesProperties.instance_not_stoppable_with_reasons.name());
+    Set<String> toBeStoppedInstancesSet = new HashSet<>(toBeStoppedInstances);
+    findToBeStoppedInstances(toBeStoppedInstancesSet);
+
+    // Because zone order calculation is omitted, we must verify each 
instance's existence
+    // to ensure we only process valid instances before performing stoppable 
check.
+    Set<String> nonExistingInstances = processNonexistentInstances(instances, 
failedStoppableInstances);

Review Comment:
   do we add back `nonExistingInstances ` to result?



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