jackjlli commented on code in PR #9309: URL: https://github.com/apache/pinot/pull/9309#discussion_r968923302
########## pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/segment/BaseSegmentAssignment.java: ########## @@ -93,62 +96,22 @@ public void init(HelixManager helixManager, TableConfig tableConfig) { protected abstract int getReplication(TableConfig tableConfig); /** - * Helper method to check whether the number of replica-groups matches the table replication for replica-group based - * instance partitions. Log a warning if they do not match and use the one inside the instance partitions. The - * mismatch can happen when table is not configured correctly (table replication and numReplicaGroups does not match - * or replication changed without reassigning instances). + * Set Segment assignment strategy for different instance partitions and puts into a map of + * Map<InstancePartitionsType, SegmentAssignmentStrategy> */ - protected void checkReplication(InstancePartitions instancePartitions) { - int numReplicaGroups = instancePartitions.getNumReplicaGroups(); - if (numReplicaGroups != _replication) { - _logger.warn( - "Number of replica-groups in instance partitions {}: {} does not match replication in table config: {} for " - + "table: {}, using: {}", instancePartitions.getInstancePartitionsName(), numReplicaGroups, _replication, - _tableNameWithType, numReplicaGroups); - } - } - - /** - * Helper method to assign instances based on the current assignment and instance partitions. - */ - protected List<String> assignSegment(String segmentName, Map<String, Map<String, String>> currentAssignment, - InstancePartitions instancePartitions) { - int numReplicaGroups = instancePartitions.getNumReplicaGroups(); - int numPartitions = instancePartitions.getNumPartitions(); - - if (numReplicaGroups == 1 && numPartitions == 1) { - // Non-replica-group based assignment - - return SegmentAssignmentUtils.assignSegmentWithoutReplicaGroup(currentAssignment, instancePartitions, - _replication); - } else { - // Replica-group based assignment - - checkReplication(instancePartitions); - - int partitionId; - if (_partitionColumn == null || numPartitions == 1) { - partitionId = 0; - } else { - // Uniformly spray the segment partitions over the instance partitions - partitionId = getSegmentPartitionId(segmentName) % numPartitions; - } - - return SegmentAssignmentUtils.assignSegmentWithReplicaGroup(currentAssignment, instancePartitions, partitionId); - } + protected void setSegmentAssignmentStrategyMap( + Map<InstancePartitionsType, InstancePartitions> instancePartitionsMap) { + _assignmentStrategyMap = SegmentAssignmentStrategyFactory + .getSegmentAssignmentStrategy(_helixManager, _tableConfig, instancePartitionsMap); Review Comment: The method name here seems a bit contradicting to what we are trying to do here. -- 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