abhioncbr commented on PR #10656: URL: https://github.com/apache/pinot/pull/10656#issuecomment-1524327572
> I don't think this is correct. We don't need to change anything in `InstanceAssignmentConfigUtils` because it will directly return the new config if it exists (line 95). `ReplicaGroupStrategyConfig.getPartitionColumn()` is used in `BaseSegmentAssignment` and `ReplicaGroupSegmentAssignmentStrategy`, so we should modify these 2 classes to be able to take the partition column from the new config Thanks for the review. Going to make the changes in `BaseSegmentAssignment` and `ReplicaGroupSegmentAssignmentStrategy`. Question for `InstanceAssignmentConfigUtils`, since we are creating an object of `InstanceReplicaGroupPartitionConfig`[L116, L121] and with this change we have added `partitionColumn` as a class member, we need to provide the value of it. I thought it's better to give the value we are extracting there instead of setting it as `null`. WDYT? -- 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
