swaminathanmanish commented on code in PR #14257:
URL: https://github.com/apache/pinot/pull/14257#discussion_r1811965906
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotTaskManager.java:
##########
@@ -589,6 +601,13 @@ private synchronized List<String> scheduleTask(String
taskType, List<String> tab
PinotTaskGenerator taskGenerator =
_taskGeneratorRegistry.getTaskGenerator(taskType);
Preconditions.checkState(taskGenerator != null, "Task type: %s is not
registered", taskType);
+ TaskState taskState =
_helixTaskResourceManager.getTaskQueueState(taskType);
+ if (TaskState.STOPPED.equals(taskState) ||
TaskState.STOPPING.equals(taskState)) {
+ LOGGER.warn("Task queue is in state: {}. Tasks won't be scheduled for
taskType: {} and tables: {}. Resume task "
+ + "queue before attempting to schedule tasks.", taskState.name(),
taskType, tables);
+ return new ArrayList<>();
+ }
Review Comment:
Looks like adhoc trigger calls createTask, so we do need it there.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotTaskManager.java:
##########
@@ -163,6 +164,9 @@ public Map<String, String> createTask(String taskType,
String tableName, @Nullab
taskConfigs.getOrDefault(MINION_INSTANCE_TAG_CONFIG,
CommonConstants.Helix.UNTAGGED_MINION_INSTANCE);
_helixTaskResourceManager.ensureTaskQueueExists(taskType);
addTaskTypeMetricsUpdaterIfNeeded(taskType);
+ if (!isTaskSchedulable(taskType, List.of(tableName))) {
Review Comment:
Could you move this to the beginning of the method?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotTaskManager.java:
##########
@@ -611,9 +613,14 @@ private synchronized List<String> scheduleTask(String
taskType, List<String> tab
@Nullable
private List<String> scheduleTask(PinotTaskGenerator taskGenerator,
List<TableConfig> enabledTableConfigs,
boolean isLeader, @Nullable String minionInstanceTagForTask) {
- LOGGER.info("Trying to schedule task type: {}, isLeader: {}",
taskGenerator.getTaskType(), isLeader);
- Map<String, List<PinotTaskConfig>> minionInstanceTagToTaskConfigs = new
HashMap<>();
String taskType = taskGenerator.getTaskType();
+ List<String> enabledTables =
+
enabledTableConfigs.stream().map(TableConfig::getTableName).collect(Collectors.toList());
+ LOGGER.info("Trying to schedule task type: {}, for tables: {}, isLeader:
{}", taskType, enabledTables, isLeader);
+ if (!isTaskSchedulable(taskType, enabledTables)) {
Review Comment:
This is applicable for all tables for one taskType right?
--
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
