[GitHub] [pinot] snleee commented on a diff in pull request #10964: Allow custom segment grouping in MergeRollupTask

["snleee (via GitHub)" <gi...@apache.org>]

snleee commented on code in PR #10964:
URL: https://github.com/apache/pinot/pull/10964#discussion_r1247496454


##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotTaskManager.java:
##########
@@ -475,6 +476,14 @@ public void registerTaskGenerator(PinotTaskGenerator taskGenerator) {
     _taskGeneratorRegistry.registerTaskGenerator(taskGenerator);
   }
 
+  /**
+   * Registers a task segment group manager.
+   * <p>This method can be used to plug in custom task segment group manager.
+   */
+  public void registerTaskSegmentGroupManager(PinotTaskSegmentGroupManager taskSegmentGroupManager) {

Review Comment:
   Instead of exposing this function for registering and setting `taskSegmentGroupManager` from the top level task manager and feed this all the way to the task generator, I can think of another approach which would not require interface changes:
   
   1. Pass the factory class name for `TaskSegmentGroupManager` as part of the task config for Merge&Rollup task. 
   2. initialize the `TaskSegmentGroupManager` object based on the factory class name
   
   
   This approach is what we use for Kafka client factory in the stream config.



-- 
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