kfaraz commented on code in PR #18039:
URL: https://github.com/apache/druid/pull/18039#discussion_r2121156148
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/batch/parallel/TaskMonitor.java:
##########
@@ -106,14 +108,14 @@ public class TaskMonitor<T extends Task,
SubTaskReportType extends SubTaskReport
@GuardedBy("startStopLock")
private boolean running = false;
- TaskMonitor(OverlordClient overlordClient, int maxRetry, int
estimatedNumSucceededTasks)
+ TaskMonitor(OverlordClient overlordClient, int maxRetry, int
estimatedNumSucceededTasks, long taskTimeoutMillis)
{
// Unlimited retries for Overlord APIs: if it goes away, we'll wait
indefinitely for it to come back.
this.overlordClient = Preconditions.checkNotNull(overlordClient,
"overlordClient")
.withRetryPolicy(StandardRetryPolicy.unlimited());
this.maxRetry = maxRetry;
this.estimatedNumSucceededTasks = estimatedNumSucceededTasks;
-
+ this.taskTimeoutMillis = taskTimeoutMillis;
log.info("TaskMonitor is initialized with estimatedNumSucceededTasks[%d]",
estimatedNumSucceededTasks);
Review Comment:
```suggestion
log.info("TaskMonitor is initialized with estimatedNumSucceededTasks[%d]
and sub-task timeout[%d millis].", estimatedNumSucceededTasks,
taskTimeoutMillis);
```
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/batch/parallel/ParallelIndexTuningConfig.java:
##########
@@ -275,6 +275,7 @@ public int getMaxAllowedLockCount()
return maxAllowedLockCount;
}
+
Review Comment:
Extra new line:
```suggestion
```
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/batch/parallel/TaskMonitor.java:
##########
@@ -134,6 +136,25 @@ public void start(long taskStatusCheckingPeriod)
final MonitorEntry monitorEntry = entry.getValue();
final String taskId = monitorEntry.runningTask.getId();
+ final long timeout = taskTimeoutMillis;
Review Comment:
This assignment seems unncessary, we can just use `taskTimeoutMillis` itself.
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/batch/parallel/TaskMonitor.java:
##########
@@ -134,6 +136,25 @@ public void start(long taskStatusCheckingPeriod)
final MonitorEntry monitorEntry = entry.getValue();
final String taskId = monitorEntry.runningTask.getId();
+ final long timeout = taskTimeoutMillis;
+ final long elapsed =
monitorEntry.getStopwatch().millisElapsed();
+ if (timeout > 0 && elapsed > timeout) {
+ log.warn("task[%s] timed out after %s ms, cancelling...",
taskId, elapsed);
+ FutureUtils.getUnchecked(overlordClient.cancelTask(taskId),
true);
+ final TaskStatusPlus cancelledTaskStatus =
FutureUtils.getUnchecked(
+ overlordClient.taskStatus(taskId), true).getStatus();
+ reportsMap.remove(taskId);
+ incrementNumFailedTasks();
+
+ if (monitorEntry.numTries() < maxRetry) {
+ retry(specId, monitorEntry, cancelledTaskStatus);
Review Comment:
I am not sure if we should retry a task that has been cancelled for
exceeding the timeout.
Doesn't retrying somewhat defeat the purpose of the timeout?
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/batch/parallel/TaskMonitor.java:
##########
@@ -134,6 +136,25 @@ public void start(long taskStatusCheckingPeriod)
final MonitorEntry monitorEntry = entry.getValue();
final String taskId = monitorEntry.runningTask.getId();
+ final long timeout = taskTimeoutMillis;
+ final long elapsed =
monitorEntry.getStopwatch().millisElapsed();
+ if (timeout > 0 && elapsed > timeout) {
+ log.warn("task[%s] timed out after %s ms, cancelling...",
taskId, elapsed);
Review Comment:
```suggestion
log.warn("Cancelling task[%s] as it has already run for
[%d] millis (taskTimeoutMillis=[%d]).", taskId, elapsed, taskTimeoutMillis);
```
--
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]
