Copilot commented on code in PR #17190:
URL: https://github.com/apache/pinot/pull/17190#discussion_r2513890635
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/TaskMetricsEmitter.java:
##########
@@ -113,21 +117,19 @@ protected final void runTask(Properties
periodicTaskProperties) {
long previousExecutionTimestamp =
_previousExecutionTimestamps.computeIfAbsent(taskType,
k -> currentExecutionTimestamp - _taskMetricsEmitterFrequencyMs);
- // Get currently in-progress tasks (for metrics)
- Set<String> currentInProgressTasks =
_helixTaskResourceManager.getTasksInProgress(taskType);
-
// Get tasks that were in-progress during the previous collection cycle
Set<String> previouslyInProgressTasks =
_previousInProgressTasks.getOrDefault(taskType,
Collections.emptySet());
- // Get all tasks including those that started after the previous
execution timestamp
- // This combines in-progress tasks and short-lived tasks that started
and completed between cycles
- // in a single Helix call, avoiding duplicate
getWorkflowConfig/getWorkflowContext calls
- Set<String> tasksIncludingShortLived =
_helixTaskResourceManager.getTasksInProgressAndRecent(
- taskType, previousExecutionTimestamp);
+ // Get in-progress tasks and all tasks to report (including
short-lived) in a single Helix call
+ TasksByStatus taskResult =
_helixTaskResourceManager.getTasksByStatus(taskType,
previousExecutionTimestamp);
+ Set<String> currentInProgressTasks = taskResult.getInProgressTasks();
+ Set<String> tasksIncludingShortLived =
taskResult.getAllTasksToReport();
// Start with all tasks that need reporting (in-progress + short-lived)
- Set<String> tasksToReport = new HashSet<>(tasksIncludingShortLived);
+ Set<String> tasksToReport = new HashSet<>();
+ tasksToReport.addAll(currentInProgressTasks);
Review Comment:
The code adds both `currentInProgressTasks` and `tasksIncludingShortLived`
to `tasksToReport`, but since `getAllTasksToReport()` already combines
in-progress and recent tasks, line 131 is redundant. Consider using
`tasksToReport.addAll(tasksIncludingShortLived)` directly to avoid duplicate
additions of in-progress tasks.
```suggestion
```
--
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]
