Pinot API for getting all minions tasks and their summary #17330
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #17330 +/- ##
============================================
- Coverage 63.28% 63.28% -0.01%
- Complexity 1434 1474 +40
============================================
Files 3134 3135 +1
Lines 186302 186599 +297
Branches 28456 28510 +54
============================================
+ Hits 117907 118087 +180
- Misses 59300 59392 +92
- Partials 9095 9120 +25
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR introduces a new /tasks/summary REST endpoint that provides a consolidated view of task information across all task types, eliminating the need for multiple API calls to gather task statistics.
Key Changes:
- New REST endpoint
GET /tasks/summarywith appropriate authorization - New response models
TaskSummaryResponseandTaskTypeBreakdownto structure the aggregated data - Implementation in
PinotHelixTaskResourceManager.getTasksSummary()that aggregates task counts from existing methods
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| PinotTaskRestletResource.java | Adds the /tasks/summary endpoint that delegates to the task resource manager |
| PinotHelixTaskResourceManager.java | Implements getTasksSummary() method and defines response model classes for task summary data |
| PinotTaskRestletResourceTest.java | Adds comprehensive test coverage for the new endpoint with three test scenarios |
...c/main/java/org/apache/pinot/controller/helix/core/minion/PinotHelixTaskResourceManager.java
Outdated
Show resolved
Hide resolved
...c/main/java/org/apache/pinot/controller/helix/core/minion/PinotHelixTaskResourceManager.java
Outdated
Show resolved
Hide resolved
...c/main/java/org/apache/pinot/controller/helix/core/minion/PinotHelixTaskResourceManager.java
Outdated
Show resolved
Hide resolved
| continue; | ||
| } | ||
|
|
||
| Map<String, String> configs = subtaskConfigs.get(0).getConfigs(); |
There was a problem hiding this comment.
The code retrieves configs from the first subtask without checking if the list is empty. Line 1028 checks subtaskConfigs.isEmpty() and continues, but get(0) at line 1032 could still throw IndexOutOfBoundsException if the list becomes empty between the check and access due to concurrent modification. Although the method is synchronized, it's safer to use a defensive pattern like storing the first config in a variable after the isEmpty check to ensure clarity and prevent potential issues.
| Map<String, String> configs = subtaskConfigs.get(0).getConfigs(); | |
| PinotTaskConfig firstSubtaskConfig = subtaskConfigs.get(0); | |
| Map<String, String> configs = firstSubtaskConfig.getConfigs(); |
| taskCount2.addTaskState(null); // null state counts as waiting | ||
| taskCount2.addTaskState(null); // null state counts as waiting |
There was a problem hiding this comment.
[nitpick] These inline comments explaining null state behavior are helpful and should be consistently applied. Similar null state additions at lines 1361, 1422-1423, and 1630 have comments, but line 1361's comment style differs. Consider standardizing the comment format across all null state additions for consistency.
| // Skip if this parent task has no running/waiting tasks (optimization: avoid table breakdown call) | ||
| if (totalTaskCount.getRunning() == 0 && totalTaskCount.getWaiting() == 0) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The comment on line 1020 is excellent as it explains both the condition and the performance rationale. However, the similar optimization at lines 1074-1079 lacks an explanatory comment about why only non-zero counts are included. Adding a comment there would improve consistency and code clarity.
...c/main/java/org/apache/pinot/controller/helix/core/minion/PinotHelixTaskResourceManager.java
Outdated
Show resolved
Hide resolved
| private static final String UNKNOWN_TABLE_NAME = "unknown"; | ||
| private static final String UNKNOWN_TENANT_NAME = "unknown"; |
There was a problem hiding this comment.
The constant UNKNOWN_TENANT_NAME duplicates the value of the existing CommonConstants.UNKNOWN constant (imported at line 65). Consider reusing CommonConstants.UNKNOWN to maintain consistency with the existing constant UNKNOWN_TABLE_NAME on line 86, which uses \"unknown\" but should ideally reference the same shared constant.
| private static final String UNKNOWN_TABLE_NAME = "unknown"; | |
| private static final String UNKNOWN_TENANT_NAME = "unknown"; | |
| private static final String UNKNOWN_TABLE_NAME = CommonConstants.UNKNOWN; |
| return serverTenant != null ? serverTenant : UNKNOWN_TENANT_NAME; | ||
| } | ||
| return UNKNOWN_TENANT_NAME; | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Catching generic Exception is overly broad and may mask unexpected errors. Consider catching more specific exception types that getTableConfig() can throw, or document why a broad catch is necessary here.
| } catch (Exception e) { | |
| } catch (IllegalStateException | NullPointerException e) { |
| PinotHelixTaskResourceManager.TaskCount taskCount = new PinotHelixTaskResourceManager.TaskCount(); | ||
| taskCount.addTaskState(TaskPartitionState.RUNNING); | ||
| taskCount.addTaskState(TaskPartitionState.RUNNING); | ||
| taskCount.addTaskState(null); // null state counts as waiting |
There was a problem hiding this comment.
The inline comment states 'null state counts as waiting', but this behavior is not immediately evident from the test setup. Consider adding a comment explaining why a null state is used to represent waiting tasks, or reference the relevant code in the implementation that defines this behavior.
| taskCount.addTaskState(null); // null state counts as waiting | |
| // In TaskCount.addTaskState, a null state is interpreted as a waiting task. | |
| // See PinotHelixTaskResourceManager.TaskCount#addTaskState for implementation details. | |
| taskCount.addTaskState(null); |
3 participants
Adds a new
/tasks/summaryREST endpoint that consolidates task information across all task types into a single API call.GET /tasks/summaryTaskSummaryResponsewith aggregated task countsAPI Response Format
{ "totalRunningTasks": 350, "totalWaitingTasks": 150, "taskBreakdown": [ { "tenant": "defaultTenant", "runningTasks": 200, "waitingTasks": 100, "taskTypeBreakdown": [ { "taskType": "SegmentGenerationAndPushTask", "runningCount": 200, "waitingCount": 100 } ] }, { "tenant": "premiumTenant", "runningTasks": 100, "waitingTasks": 30, "taskTypeBreakdown": [ { "taskType": "SegmentGenerationAndPushTask", "runningCount": 70, "waitingCount": 20 }, { "taskType": "MergeRollupTask", "runningCount": 30, "waitingCount": 10 } ] }, { "tenant": "analyticsTenant", "runningTasks": 50, "waitingTasks": 20, "taskTypeBreakdown": [ { "taskType": "UpsertCompactionTask", "runningCount": 50, "waitingCount": 20 } ] } ] }