Incorported Review comments

LZD-PratyushBhatt · LZD-PratyushBhatt · commit f11bc8aaa42f · 2025-05-01T03:26:33.000+05:30
diff --git a/helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/StoppableInstancesSelector.java b/helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/StoppableInstancesSelector.java
@@ -63,19 +63,16 @@ public class StoppableInstancesSelector {
   private final MaintenanceManagementService _maintenanceService;
   private final ClusterTopology _clusterTopology;
   private final ZKHelixDataAccessor _dataAccessor;
-  private final boolean _preserveOrder;
 
   private StoppableInstancesSelector(String clusterId, List<String> orderOfZone,
       String customizedInput, MaintenanceManagementService maintenanceService,
-      ClusterTopology clusterTopology, ZKHelixDataAccessor dataAccessor,
-      boolean preserveOrder) {
+      ClusterTopology clusterTopology, ZKHelixDataAccessor dataAccessor) {
     _clusterId = clusterId;
     _orderOfZone = orderOfZone;
     _customizedInput = customizedInput;
     _maintenanceService = maintenanceService;
     _clusterTopology = clusterTopology;
     _dataAccessor = dataAccessor;
-    _preserveOrder = preserveOrder;
   }
 
   /**
@@ -86,14 +83,15 @@ private StoppableInstancesSelector(String clusterId, List<String> orderOfZone,
    *
    * @param instances A list of instance to be evaluated.
    * @param toBeStoppedInstances A list of instances presumed to be already stopped
+   * @param preserveOrder Indicates whether to preserve the original order of instances
    * @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 getZoneBasedInstancesreasons for non-stoppability as the value.
    * @throws IOException
    */
   public ObjectNode getStoppableInstancesInSingleZone(List<String> instances,
-      List<String> toBeStoppedInstances) throws IOException {
+      List<String> toBeStoppedInstances, boolean preserveOrder) throws IOException {
     ObjectNode result = JsonNodeFactory.instance.objectNode();
     ArrayNode stoppableInstances =
         result.putArray(InstancesAccessor.InstancesProperties.instance_stoppable_parallel.name());
@@ -102,9 +100,9 @@ public ObjectNode getStoppableInstancesInSingleZone(List<String> instances,
     Set<String> toBeStoppedInstancesSet = findToBeStoppedInstances(toBeStoppedInstances);
 
     List<String> zoneBasedInstance =
-        getZoneBasedInstances(instances, _clusterTopology.toZoneMapping());
+        getZoneBasedInstances(instances, _clusterTopology.toZoneMapping(), preserveOrder);
     populateStoppableInstances(zoneBasedInstance, toBeStoppedInstancesSet, stoppableInstances,
-        failedStoppableInstances, _preserveOrder);
+        failedStoppableInstances, preserveOrder);
     processNonexistentInstances(instances, failedStoppableInstances);
 
     return result;
@@ -117,6 +115,7 @@ public ObjectNode getStoppableInstancesInSingleZone(List<String> instances,
    *
    * @param instances A list of instance to be evaluated.
    * @param toBeStoppedInstances A list of instances presumed to be already stopped
+   * @param preserveOrder Indicates whether to preserve the original order of instances
    * @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
@@ -154,6 +153,7 @@ public ObjectNode getStoppableInstancesCrossZones(List<String> instances,
    *
    * @param instances A list of instance to be evaluated.
    * @param toBeStoppedInstances A list of instances presumed to be already stopped
+   * @param preserveOrder Indicates whether to preserve the original order of instances
    * @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
@@ -266,36 +266,37 @@ public void calculateOrderOfZone(List<String> instances, boolean random) {
    *
    * @param instances List of instances to be considered
    * @param zoneMapping Mapping from zone to instances
+   * @param preserveOrder Indicates whether to preserve the original order of instances
    * @return List of instances in the first non-empty zone. If preserveOrder is true, the original order
    *         of instances is maintained. If preserveOrder is false (default), instances are sorted lexicographically.
    */
   private List<String> getZoneBasedInstances(List<String> instances,
-      Map<String, Set<String>> zoneMapping) {
+      Map<String, Set<String>> zoneMapping, boolean preserveOrder) {
     if (_orderOfZone.isEmpty()) {
-      return _orderOfZone;
+      return Collections.emptyList();
     }
 
-    Set<String> instanceSet = null;
     for (String zone : _orderOfZone) {
-      if (_preserveOrder) {
-        List<String> filteredInstances = new ArrayList<>(instances);
-        Set<String> currentZoneInstanceSet = new HashSet<>(zoneMapping.get(zone));
-        filteredInstances.removeIf(instance -> !currentZoneInstanceSet.contains(instance));
-        if (!filteredInstances.isEmpty()) {
-          return filteredInstances;
-        }
-      } else {
-        // Original behavior - lexicographical ordering via TreeSet
-        instanceSet = new TreeSet<>(instances);
-        Set<String> currentZoneInstanceSet = new HashSet<>(zoneMapping.get(zone));
-        instanceSet.retainAll(currentZoneInstanceSet);
-        if (!instanceSet.isEmpty()) {
-          return new ArrayList<>(instanceSet);
+      Set<String> currentZoneInstanceSet = zoneMapping.get(zone);
+      if (currentZoneInstanceSet == null || currentZoneInstanceSet.isEmpty()) {
+        continue;
+      }
+
+      // Filter instances based on current zone
+      List<String> filteredInstances = instances.stream()
+          .filter(currentZoneInstanceSet::contains)
+          .collect(Collectors.toList());
+
+      if (!filteredInstances.isEmpty()) {
+        // If preserve order is not required, return sorted list
+        if (!preserveOrder) {
+          Collections.sort(filteredInstances); // Lexicographical order
         }
+        return filteredInstances;
       }
     }
 
-    return Collections.EMPTY_LIST;
+    return Collections.emptyList();
   }
 
   /**
@@ -343,7 +344,6 @@ public static class StoppableInstancesSelectorBuilder {
     private MaintenanceManagementService _maintenanceService;
     private ClusterTopology _clusterTopology;
     private ZKHelixDataAccessor _dataAccessor;
-    private boolean _preserveOrder = false; // Default to false for backward compatibility
 
     public StoppableInstancesSelectorBuilder setClusterId(String clusterId) {
       _clusterId = clusterId;
@@ -376,14 +376,9 @@ public StoppableInstancesSelectorBuilder setDataAccessor(ZKHelixDataAccessor dat
       return this;
     }
 
-    public StoppableInstancesSelectorBuilder setPreserveOrder(boolean preserveOrder) {
-      _preserveOrder = preserveOrder;
-      return this;
-    }
-
     public StoppableInstancesSelector build() {
       return new StoppableInstancesSelector(_clusterId, _orderOfZone, _customizedInput,
-          _maintenanceService, _clusterTopology, _dataAccessor, _preserveOrder);
+          _maintenanceService, _clusterTopology, _dataAccessor);
     }
   }
 }
diff --git a/helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/InstancesAccessor.java b/helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/InstancesAccessor.java
@@ -78,7 +78,6 @@ public enum InstancesProperties {
     to_be_stopped_instances,
     skip_stoppable_check_list,
     customized_values,
-    preserve_order,
     instance_stoppable_parallel,
     instance_not_stoppable_with_reasons
   }
@@ -160,7 +159,9 @@ public Response instancesOperations(@PathParam("clusterId") String clusterId,
       @QueryParam("continueOnFailures") boolean continueOnFailures,
       @QueryParam("skipZKRead") boolean skipZKRead,
       @QueryParam("skipHealthCheckCategories") String skipHealthCheckCategories,
-      @DefaultValue("false") @QueryParam("random") boolean random, String content) {
+      @DefaultValue("false") @QueryParam("random") boolean random,
+      @DefaultValue("false") @QueryParam("preserveOrder") boolean preserveOrder,
+      String content) {
     Command cmd;
     try {
       cmd = Command.valueOf(command);
@@ -205,7 +206,7 @@ public Response instancesOperations(@PathParam("clusterId") String clusterId,
           break;
         case stoppable:
           return batchGetStoppableInstances(clusterId, node, skipZKRead, continueOnFailures,
-              skipHealthCheckCategorySet, random);
+              skipHealthCheckCategorySet, random, preserveOrder);
         default:
           _logger.error("Unsupported command :" + command);
           return badRequest("Unsupported command :" + command);
@@ -223,7 +224,7 @@ public Response instancesOperations(@PathParam("clusterId") String clusterId,
 
   private Response batchGetStoppableInstances(String clusterId, JsonNode node, boolean skipZKRead,
       boolean continueOnFailures, Set<StoppableCheck.Category> skipHealthCheckCategories,
-      boolean random) throws IOException {
+      boolean random, boolean preserveOrder) throws IOException {
     try {
       // TODO: Process input data from the content
       // TODO: Implement the logic to automatically detect the selection base. https://github.com/apache/helix/issues/2968#issue-2691677799
@@ -305,11 +306,6 @@ private Response batchGetStoppableInstances(String clusterId, JsonNode node, boo
             .asBoolean();
       }
 
-      boolean preserveOrder = false;
-      if (node.get(InstancesProperties.preserve_order.name()) != null) {
-        preserveOrder = node.get(InstancesProperties.preserve_order.name()).asBoolean();
-      }
-
       ClusterTopology clusterTopology = clusterService.getClusterTopology(clusterId);
       if (selectionBase != InstanceHealthSelectionBase.non_zone_based) {
         if (!clusterService.isClusterTopologyAware(clusterId)) {
@@ -360,14 +356,13 @@ private Response batchGetStoppableInstances(String clusterId, JsonNode node, boo
               .setMaintenanceService(maintenanceService)
               .setClusterTopology(clusterTopology)
               .setDataAccessor((ZKHelixDataAccessor) getDataAccssor(clusterId))
-              .setPreserveOrder(preserveOrder)
               .build();
       ObjectNode result;
 
       switch (selectionBase) {
         case zone_based:
           stoppableInstancesSelector.calculateOrderOfZone(instances, random);
-          result = stoppableInstancesSelector.getStoppableInstancesInSingleZone(instances, toBeStoppedInstances);
+          result = stoppableInstancesSelector.getStoppableInstancesInSingleZone(instances, toBeStoppedInstances, preserveOrder);
           break;
         case cross_zone_based:
           stoppableInstancesSelector.calculateOrderOfZone(instances, random);
diff --git a/helix-rest/src/test/java/org/apache/helix/rest/server/TestInstancesAccessor.java b/helix-rest/src/test/java/org/apache/helix/rest/server/TestInstancesAccessor.java
@@ -659,15 +659,16 @@ public void testMultipleReplicasInSameMZWithPreserveOrder(boolean preserveOrder)
 
     // Run stoppable check against the 3 instances where SemiAuto DB was assigned
     String content =
-        String.format("{\"%s\":\"%s\",\"%s\":[\"%s\",\"%s\",\"%s\"],\"%s\":%s}",
+        String.format("{\"%s\":\"%s\",\"%s\":[\"%s\",\"%s\",\"%s\"]}",
             InstancesAccessor.InstancesProperties.selection_base.name(),
             InstancesAccessor.InstanceHealthSelectionBase.zone_based.name(),
-            InstancesAccessor.InstancesProperties.instances.name(), "instance1", "instance2", "instance0",
-            InstancesAccessor.InstancesProperties.preserve_order.name(),
-            preserveOrder);
-    Response response =
-        new JerseyUriRequestBuilder("clusters/{}/instances?command=stoppable&skipHealthCheckCategories=CUSTOM_INSTANCE_CHECK,CUSTOM_PARTITION_CHECK").format(
-            STOPPABLE_CLUSTER2).post(this, Entity.entity(content, MediaType.APPLICATION_JSON_TYPE));
+            InstancesAccessor.InstancesProperties.instances.name(), "instance1", "instance2", "instance0");
+    Response response = new JerseyUriRequestBuilder(String.format(
+        "clusters/%s/instances?command=stoppable&skipHealthCheckCategories=%s&preserveOrder=%s",
+        STOPPABLE_CLUSTER2,
+        "CUSTOM_INSTANCE_CHECK,CUSTOM_PARTITION_CHECK",
+        preserveOrder))
+        .post(this, Entity.entity(content, MediaType.APPLICATION_JSON_TYPE));
     JsonNode jsonNode = OBJECT_MAPPER.readTree(response.readEntity(String.class));
 
     String stoppableNode = "instance0";
