Copilot commented on code in PR #17190:
URL: https://github.com/apache/pinot/pull/17190#discussion_r2516439276
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotHelixTaskResourceManager.java:
##########
@@ -1336,6 +1364,31 @@ public PinotTaskConfig getTaskConfig() {
}
}
+ /**
+ * Result class that organizes tasks by status.
+ * Holds sets of tasks directly, making it extensible for future use cases.
+ */
+ public static class TasksByStatus {
+ private Set<String> _inProgressTasks = Collections.emptySet();
+ private Set<String> _recentTasks = Collections.emptySet(); // Tasks that
started after timestamp (any state)
Review Comment:
[nitpick] Initializing fields with `Collections.emptySet()` creates
immutable sets. When `setInProgressTasks()` or `setRecentTasks()` is called,
the reference is replaced, which works correctly. However, this pattern can be
confusing as it suggests the fields might be mutable empty sets. Consider
initializing to `new HashSet<>()` for clarity and consistency with typical
JavaBean patterns, or document that these fields are meant to be replaced
rather than mutated.
```suggestion
private Set<String> _inProgressTasks = new HashSet<>();
private Set<String> _recentTasks = new HashSet<>(); // Tasks that
started after timestamp (any state)
```
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotHelixTaskResourceManager.java:
##########
@@ -1435,4 +1488,34 @@ public void accumulate(TaskCount other) {
_aborted += other.getAborted();
}
}
+
+ 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
Review Comment:
The `TaskStatusSummary` class lacks a class-level Javadoc comment explaining
its purpose and relationship to `TaskCount`. Add documentation describing that
this class consolidates task state counts with timing information for subtasks,
and explain when timing maps may be populated versus empty.
--
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]
