jackjlli commented on a change in pull request #8441: URL: https://github.com/apache/pinot/pull/8441#discussion_r839295778
########## File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/instance/InstanceTagPoolSelector.java ########## @@ -48,52 +51,108 @@ public InstanceTagPoolSelector(InstanceTagPoolConfig tagPoolConfig, String table /** * Returns a map from pool to instance configs based on the tag and pool config for the given instance configs. + * @param instanceConfigs list of latest instance configs from ZK. + * @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 Map<Integer, List<InstanceConfig>> selectInstances(List<InstanceConfig> instanceConfigs) { + public Map<Integer, List<InstanceConfig>> selectInstances(List<InstanceConfig> instanceConfigs, + Map<Integer, List<String>> partitionToInstancesMap) { int tableNameHash = Math.abs(_tableNameWithType.hashCode()); LOGGER.info("Starting instance tag/pool selection for table: {} with hash: {}", _tableNameWithType, tableNameHash); - // Filter out the instances with the correct tag + // Filter out the instances with the correct tag. String tag = _tagPoolConfig.getTag(); - List<InstanceConfig> candidateInstanceConfigs = new ArrayList<>(); + Map<String, InstanceConfig> candidateInstanceConfigsMap = new LinkedHashMap<>(); for (InstanceConfig instanceConfig : instanceConfigs) { if (instanceConfig.getTags().contains(tag)) { - candidateInstanceConfigs.add(instanceConfig); + candidateInstanceConfigsMap.put(instanceConfig.getInstanceName(), instanceConfig); } } - candidateInstanceConfigs.sort(Comparator.comparing(InstanceConfig::getInstanceName)); - int numCandidateInstances = candidateInstanceConfigs.size(); + + // Find out newly added instances from the latest copies of instance configs. + Deque<String> newlyAddedInstances = new LinkedList<>(candidateInstanceConfigsMap.keySet()); + for (List<String> existingInstancesWithSequence : partitionToInstancesMap.values()) { + newlyAddedInstances.removeAll(existingInstancesWithSequence); + } + + int numCandidateInstances = candidateInstanceConfigsMap.size(); Preconditions.checkState(numCandidateInstances > 0, "No enabled instance has the tag: %s", tag); LOGGER.info("{} enabled instances have the tag: {} for table: {}", numCandidateInstances, tag, _tableNameWithType); - Map<Integer, List<InstanceConfig>> poolToInstanceConfigsMap = new TreeMap<>(); + // Each pool number associates with a map that key is the instance name and value is the instance config. + Map<Integer, Map<String, InstanceConfig>> poolToInstanceConfigsMap = new HashMap<>(); Review comment: The purpose of storing instanceName as the key of the inner map is fast retrieve the instance config given the instance name in a pool. Say in Pool 0, I want to get the instance config given a new instance name from `newlyAddedInstances`, then I can fetch it in O(1) time and assign it to the target pool. -- 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-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org