[GitHub] [pinot] siddharthteotia commented on a change in pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances

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

siddharthteotia commented on a change in pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#discussion_r839022456



##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/instance/InstanceAssignmentDriver.java
##########
@@ -51,16 +52,34 @@ public InstanceAssignmentDriver(TableConfig tableConfig) {
     _tableConfig = tableConfig;
   }
 
+  /**
+   * Assign instances to InstancePartitions object.
+   * @param instancePartitionsType type of instance partitions
+   * @param instanceConfigs list of instance configs
+   * @param partitionToInstancesMap existing instance with sequence that should be respected. An empty list
+   *                                      means no preceding sequence to respect and the instances would be sorted.
+   */
   public InstancePartitions assignInstances(InstancePartitionsType instancePartitionsType,
-      List<InstanceConfig> instanceConfigs) {
+      List<InstanceConfig> instanceConfigs, Map<Integer, List<String>> partitionToInstancesMap) {
+    boolean shouldRetainInstanceSequence = !partitionToInstancesMap.isEmpty();
     String tableNameWithType = _tableConfig.getTableName();
-    LOGGER.info("Starting {} instance assignment for table: {}", instancePartitionsType, tableNameWithType);
+    LOGGER.info("Starting {} instance assignment for table: {}. Should retain instance sequence: {}",
+        instancePartitionsType, tableNameWithType, shouldRetainInstanceSequence);
 
     InstanceAssignmentConfig assignmentConfig =
         InstanceAssignmentConfigUtils.getInstanceAssignmentConfig(_tableConfig, instancePartitionsType);
     InstanceTagPoolSelector tagPoolSelector =
         new InstanceTagPoolSelector(assignmentConfig.getTagPoolConfig(), tableNameWithType);
-    Map<Integer, List<InstanceConfig>> poolToInstanceConfigsMap = tagPoolSelector.selectInstances(instanceConfigs);
+    Map<Integer, List<InstanceConfig>> poolToInstanceConfigsMap =
+        tagPoolSelector.selectInstances(instanceConfigs, partitionToInstancesMap);
+
+    InstancePartitions instancePartitions = new InstancePartitions(
+        instancePartitionsType.getInstancePartitionsName(TableNameBuilder.extractRawTableName(tableNameWithType)));
+    if (shouldRetainInstanceSequence) {
+      // Keep the partition to instances map if instance sequence should be retained.
+      instancePartitions
+          .setPartitionToInstancesMap(extractInstanceNamesFromPoolToInstanceConfigsMap(poolToInstanceConfigsMap));
+    }
 
     InstanceConstraintConfig constraintConfig = assignmentConfig.getConstraintConfig();

Review comment:
       Did we think about wrapping this under a new concrete implementation of `InstanceConstraintApplier`
   
   The current constraint applier rotates the instances to alleviate hotspots but that happens after the `tagPoolSelector` has already selected the candidate instances and sorted them. 
   
   Can handle the requirement in this PR by moving the constraint applier inside the `tagPoolSelector` ?




-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org