GSharayu commented on code in PR #9309: URL: https://github.com/apache/pinot/pull/9309#discussion_r970324574
########## 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: Yes, Initially we were setting assignment strategy in Offline and Realtime segment assignment and getting that from factory and the naming was misaligned. We have refactored that part of code. So naming will now not be conflicting -- 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