junkaixue commented on code in PR #3029:
URL: https://github.com/apache/helix/pull/3029#discussion_r2069417507
##########
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/InstancesAccessor.java:
##########
@@ -78,6 +78,7 @@ public enum InstancesProperties {
to_be_stopped_instances,
skip_stoppable_check_list,
customized_values,
+ preserve_order,
Review Comment:
It is confusing as this becomes part of InstanceProperties. Preserve order
should be instance pool level property.
##########
helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/StoppableInstancesSelector.java:
##########
@@ -319,6 +343,7 @@ public static class StoppableInstancesSelectorBuilder {
private MaintenanceManagementService _maintenanceService;
private ClusterTopology _clusterTopology;
private ZKHelixDataAccessor _dataAccessor;
+ private boolean _preserveOrder = false; // Default to false for backward
compatibility
Review Comment:
This is not a good idea to keep this here. I am more leaning towards this to
be part of request param as per request thing instead of memorizing as service
setup.
##########
helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/StoppableInstancesSelector.java:
##########
@@ -263,11 +277,21 @@ private List<String> getZoneBasedInstances(List<String>
instances,
Set<String> instanceSet = null;
for (String zone : _orderOfZone) {
- instanceSet = new TreeSet<>(instances);
- Set<String> currentZoneInstanceSet = new
HashSet<>(zoneMapping.get(zone));
- instanceSet.retainAll(currentZoneInstanceSet);
- if (instanceSet.size() > 0) {
- return new ArrayList<>(instanceSet);
+ 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);
+ }
Review Comment:
This is not something good for future maintenance and code readability.
Fundamentally, it branches the logic into two.
Please combine them together with minimized section for guaranteeing the
original ordering.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
