xiangfu0 commented on code in PR #20039:
URL: https://github.com/apache/pinot/pull/20039#discussion_r4372651148
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotHelixTaskResourceManager.java:
##########
@@ -1814,6 +1828,8 @@ public static class TaskStatusSummary {
private TaskCount _taskCount = new TaskCount();
private Map<String, Long> _subtaskWaitingTimes = new HashMap<>(); //
subtask ID -> waiting time in milliseconds
private Map<String, Long> _subtaskRunningTimes = new HashMap<>(); //
subtask ID -> running time in milliseconds
+ // Time since a subtask of the job last started or finished; 0 when the
table has no waiting subtask
Review Comment:
Small doc nit: the comment says the field is `0 when the table has no
waiting subtask`, which holds because of the
`getSubtaskWaitingTimes().isEmpty()` guard, not because of anything on the
field itself. Might read more clearly as "left at 0 unless the table has a
waiting subtask" so the invariant is tied to where it's actually enforced.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotHelixTaskResourceManager.java:
##########
@@ -542,6 +544,8 @@ public synchronized Map<String, TaskStatusSummary>
getTableTaskStatusSummary(Str
if (partition != null) {
state = jobContext.getPartitionState(partition);
executionStartTime = jobContext.getPartitionStartTime(partition);
+ lastProgressTime =
+ Math.max(lastProgressTime, Math.max(executionStartTime,
jobContext.getPartitionFinishTime(partition)));
Review Comment:
`lastProgressTime` is accumulated across every partition in the job
regardless of which table it belongs to, but a single Helix job can contain
subtasks for multiple tables (your new test exercises exactly that with table1
+ table2). Below, this one job-wide value gets written to every table that has
a waiting subtask.
So if table1 is draining fast but table2 is genuinely stuck, table1's
progress keeps `lastProgressTime` recent and table2's stall gauge reads ~0 —
which is the opposite of what this gauge is meant to catch. Consider tracking
`lastProgressTime` per table (a `Map<String,Long>`) so each table's stall
reflects only its own subtasks.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotHelixTaskResourceManager.java:
##########
@@ -562,6 +566,16 @@ public synchronized Map<String, TaskStatusSummary>
getTableTaskStatusSummary(Str
taskStatusSummary.getSubtaskRunningTimes().put(taskId,
runningTimeMillis);
}
}
+ // A job whose subtasks keep starting or finishing is draining, however
old it is: only the time since the last
+ // such progress tells a stuck queue apart from a long one
+ if (jobStartTime > 0) {
+ long queueStallTimeMillis = currentTime - lastProgressTime;
+ for (TaskStatusSummary taskStatusSummary :
taskStatusSummaryMap.values()) {
+ if (!taskStatusSummary.getSubtaskWaitingTimes().isEmpty()) {
+ taskStatusSummary.setQueueStallTimeMs(queueStallTimeMillis);
Review Comment:
Since `lastProgressTime` is job-wide here, this loop applies the same stall
value to all waiting tables. If you move to per-table tracking (see the note
above) this becomes a straightforward per-table lookup and the semantics line
up with the per-table gauge.
--
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]
