Jackie-Jiang commented on code in PR #12459:
URL: https://github.com/apache/pinot/pull/12459#discussion_r1508329145
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotTaskManager.java:
##########
@@ -534,66 +535,88 @@ private synchronized Map<String, String>
scheduleTasks(List<String> tableNamesWi
* Returns the task name, or {@code null} if no task is scheduled.
*/
@Nullable
- private String scheduleTask(PinotTaskGenerator taskGenerator,
List<TableConfig> enabledTableConfigs,
+ private List<String> scheduleTask(PinotTaskGenerator taskGenerator,
List<TableConfig> enabledTableConfigs,
boolean isLeader) {
LOGGER.info("Trying to schedule task type: {}, isLeader: {}",
taskGenerator.getTaskType(), isLeader);
- List<PinotTaskConfig> pinotTaskConfigs;
- try {
- /* TODO taskGenerator may skip generating tasks for some of the tables
being passed to it.
- In that case, we should not be storing success timestamps for those
table. Same with exceptions that should
- only be associated with the table for which it was raised and not
every eligible table. We can have the
- generateTasks() return a list of TaskGeneratorMostRecentRunInfo for
each table
- */
- pinotTaskConfigs = taskGenerator.generateTasks(enabledTableConfigs);
- long successRunTimestamp = System.currentTimeMillis();
- for (TableConfig tableConfig : enabledTableConfigs) {
-
_taskManagerStatusCache.saveTaskGeneratorInfo(tableConfig.getTableName(),
taskGenerator.getTaskType(),
+ Map<String, List<PinotTaskConfig>> pinotMinionInstanceToTaskConfigs = new
HashMap<>();
+ String taskType = taskGenerator.getTaskType();
+ for (TableConfig tableConfig : enabledTableConfigs) {
+ String tableName = tableConfig.getTableName();
+ try {
+ String minionInstanceTag =
taskGenerator.getMinionInstanceTag(tableConfig);
+ List<PinotTaskConfig> pinotTaskConfig =
taskGenerator.generateTasks(List.of(tableConfig));
Review Comment:
This changes the task generation logic to processing one table at a time,
which means we cannot apply constraints across tables (e.g. limit total number
of tasks for a given task type). One way to solve this is to add an API `void
generateTasks(TableConfig tableConfig, List<PinotTaskConfig> taskConfigs)`,
where we also pass in the already generated task configs. When new task config
is generated, we directly append it to this list.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTaskRestletResource.java:
##########
@@ -667,13 +667,12 @@ public Map<String, Object> getCronSchedulerJobDetails(
@Produces(MediaType.APPLICATION_JSON)
@Authenticate(AccessType.UPDATE)
@ApiOperation("Schedule tasks and return a map from task type to task name
scheduled")
- public Map<String, String> scheduleTasks(@ApiParam(value = "Task type")
@QueryParam("taskType") String taskType,
+ public Map<String, List<String>> scheduleTasks(@ApiParam(value = "Task
type") @QueryParam("taskType") String taskType,
Review Comment:
This is backward incompatible. I guess we can return a comma separated
string instead so that the response is still compatible
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotTaskManager.java:
##########
@@ -534,66 +535,88 @@ private synchronized Map<String, String>
scheduleTasks(List<String> tableNamesWi
* Returns the task name, or {@code null} if no task is scheduled.
*/
@Nullable
- private String scheduleTask(PinotTaskGenerator taskGenerator,
List<TableConfig> enabledTableConfigs,
+ private List<String> scheduleTask(PinotTaskGenerator taskGenerator,
List<TableConfig> enabledTableConfigs,
boolean isLeader) {
LOGGER.info("Trying to schedule task type: {}, isLeader: {}",
taskGenerator.getTaskType(), isLeader);
- List<PinotTaskConfig> pinotTaskConfigs;
- try {
- /* TODO taskGenerator may skip generating tasks for some of the tables
being passed to it.
- In that case, we should not be storing success timestamps for those
table. Same with exceptions that should
- only be associated with the table for which it was raised and not
every eligible table. We can have the
- generateTasks() return a list of TaskGeneratorMostRecentRunInfo for
each table
- */
- pinotTaskConfigs = taskGenerator.generateTasks(enabledTableConfigs);
- long successRunTimestamp = System.currentTimeMillis();
- for (TableConfig tableConfig : enabledTableConfigs) {
-
_taskManagerStatusCache.saveTaskGeneratorInfo(tableConfig.getTableName(),
taskGenerator.getTaskType(),
+ Map<String, List<PinotTaskConfig>> pinotMinionInstanceToTaskConfigs = new
HashMap<>();
+ String taskType = taskGenerator.getTaskType();
+ for (TableConfig tableConfig : enabledTableConfigs) {
+ String tableName = tableConfig.getTableName();
+ try {
+ String minionInstanceTag =
taskGenerator.getMinionInstanceTag(tableConfig);
+ List<PinotTaskConfig> pinotTaskConfig =
taskGenerator.generateTasks(List.of(tableConfig));
+ List<PinotTaskConfig> presentTaskConfig =
pinotMinionInstanceToTaskConfigs.get(minionInstanceTag);
+ if (presentTaskConfig == null || presentTaskConfig.isEmpty()) {
+ presentTaskConfig = new ArrayList<>();
+ }
+ presentTaskConfig.addAll(pinotTaskConfig);
+ pinotMinionInstanceToTaskConfigs.put(minionInstanceTag,
presentTaskConfig);
Review Comment:
```suggestion
List<PinotTaskConfig> presentTaskConfig =
pinotMinionInstanceToTaskConfigs.computeIfAbsent(minionInstanceTag, k -> new
ArrayList<>());
presentTaskConfig.addAll(pinotTaskConfig);
```
--
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
