Copilot commented on code in PR #16571:
URL: https://github.com/apache/pinot/pull/16571#discussion_r2272255532
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotTaskManager.java:
##########
@@ -738,7 +755,36 @@ protected TaskSchedulingInfo
scheduleTask(PinotTaskGenerator taskGenerator, List
: taskGenerator.getMinionInstanceTag(tableConfig);
List<PinotTaskConfig> presentTaskConfig =
minionInstanceTagToTaskConfigs.computeIfAbsent(minionInstanceTag,
k -> new ArrayList<>());
+
+ // Update the task config with the triggeredBy information
+ // This can be used by the generator to appropriately set the subtask
configs
+ // Example usage in BaseTaskGenerator.getNumSubTasks()
+ TableTaskConfig tableTaskConfig = tableConfig.getTaskConfig();
+ if (tableTaskConfig != null &&
tableTaskConfig.isTaskTypeEnabled(taskType)) {
+
tableTaskConfig.getConfigsForTaskType(taskType).put(MinionConstants.TRIGGERED_BY,
triggeredBy);
+ }
+
taskGenerator.generateTasks(List.of(tableConfig), presentTaskConfig);
+ int maxNumberOfSubTasks = taskGenerator.getMaxAllowedSubTasksPerTask();
+ if (presentTaskConfig.size() > maxNumberOfSubTasks) {
+ String message = String.format(
+ "Number of tasks generated for task type: %s for table: %s is
%d, which is greater than the "
+ + "maximum number of tasks to schedule: %d. This is "
+ + "controlled by the cluster config %s which is set based on
controller's performance", taskType,
+ tableName, presentTaskConfig.size(), maxNumberOfSubTasks,
MinionConstants.MAX_ALLOWED_SUB_TASKS_KEY);
+ if (TaskSchedulingContext.isUserTriggeredTask(triggeredBy)) {
+ presentTaskConfig.clear();
+ // If the task is user-triggered, we throw an exception to notify
the user
+ // This is to ensure that the user is aware of the task generation
limit
+ throw new RuntimeException(message);
+ }
+ // For scheduled tasks, we log a warning and limit the number of
tasks
+ LOGGER.warn(message + "Only the first {} tasks will be scheduled",
maxNumberOfSubTasks);
+ presentTaskConfig = new ArrayList<>(presentTaskConfig.subList(0,
maxNumberOfSubTasks));
Review Comment:
The reassignment of 'presentTaskConfig' to a new ArrayList doesn't update
the reference in the 'minionInstanceTagToTaskConfigs' map. The map will still
contain the original full list, not the truncated one.
```suggestion
presentTaskConfig.subList(maxNumberOfSubTasks,
presentTaskConfig.size()).clear();
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
