Copilot commented on code in PR #17330:
URL: https://github.com/apache/pinot/pull/17330#discussion_r2600113555
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotHelixTaskResourceManager.java:
##########
@@ -958,6 +960,142 @@ private boolean hasTasksForTable(String taskName, String
tableNameWithType) {
}
}
+ /**
+ * Get the server tenant name for a given table by looking up its
configuration.
+ * Returns "unknown" if the table or tenant cannot be determined.
+ *
+ * @param tableName Table name with type (e.g., "myTable_OFFLINE")
+ * @return Server tenant name or "unknown"
+ */
+ private String getTenantForTable(String tableName) {
+ if (tableName == null || UNKNOWN_TABLE_NAME.equals(tableName)) {
+ return UNKNOWN_TENANT_NAME;
+ }
+
+ try {
+ TableConfig tableConfig =
_helixResourceManager.getTableConfig(tableName);
+ if (tableConfig != null && tableConfig.getTenantConfig() != null) {
+ String serverTenant = tableConfig.getTenantConfig().getServer();
+ return serverTenant != null ? serverTenant : UNKNOWN_TENANT_NAME;
+ }
+ return UNKNOWN_TENANT_NAME;
+ } catch (Exception e) {
Review 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.
```suggestion
} catch (IllegalStateException | NullPointerException e) {
```
##########
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/minion/PinotHelixTaskResourceManagerTest.java:
##########
@@ -1273,4 +1277,687 @@ private void mockTaskJobConfigAndContext(TaskDriver
taskDriver, String taskName,
when(jobContext.getTaskIdPartitionMap()).thenReturn(taskIdPartitionMap);
when(jobContext.getPartitionState(0)).thenReturn(state);
}
+
+ @Test
+ public void testGetTasksSummaryWithEmptyTaskTypes() {
+ TaskDriver taskDriver = mock(TaskDriver.class);
+ PinotHelixResourceManager helixResourceManager =
mock(PinotHelixResourceManager.class);
+ PinotHelixTaskResourceManager mgr = new
PinotHelixTaskResourceManager(helixResourceManager, taskDriver);
+
+ PinotHelixTaskResourceManager spyMgr = Mockito.spy(mgr);
+ when(spyMgr.getTaskTypes()).thenReturn(Collections.emptySet());
+
+ PinotHelixTaskResourceManager.TaskSummaryResponse response =
spyMgr.getTasksSummary(null);
+
+ assertNotNull(response);
+ assertEquals(response.getTotalRunningTasks(), 0);
+ assertEquals(response.getTotalWaitingTasks(), 0);
+ assertTrue(response.getTaskBreakdown().isEmpty());
+ }
+
+ @Test
+ public void testGetTasksSummaryWithNullTaskTypes() {
+ TaskDriver taskDriver = mock(TaskDriver.class);
+ PinotHelixResourceManager helixResourceManager =
mock(PinotHelixResourceManager.class);
+ PinotHelixTaskResourceManager mgr = new
PinotHelixTaskResourceManager(helixResourceManager, taskDriver);
+
+ PinotHelixTaskResourceManager spyMgr = Mockito.spy(mgr);
+ when(spyMgr.getTaskTypes()).thenReturn(null);
+
+ PinotHelixTaskResourceManager.TaskSummaryResponse response =
spyMgr.getTasksSummary(null);
+
+ assertNotNull(response);
+ assertEquals(response.getTotalRunningTasks(), 0);
+ assertEquals(response.getTotalWaitingTasks(), 0);
+ assertTrue(response.getTaskBreakdown().isEmpty());
+ }
+
+ @Test
+ public void testGetTasksSummaryWithNoActiveTasks() {
+ TaskDriver taskDriver = mock(TaskDriver.class);
+ PinotHelixResourceManager helixResourceManager =
mock(PinotHelixResourceManager.class);
+ PinotHelixTaskResourceManager mgr = new
PinotHelixTaskResourceManager(helixResourceManager, taskDriver);
+
+ String taskType = "TestTask";
+ String taskName = "Task_TestTask_12345";
+
+ PinotHelixTaskResourceManager spyMgr = Mockito.spy(mgr);
+ when(spyMgr.getTaskTypes()).thenReturn(Collections.singleton(taskType));
+
+ // Task with no running/waiting tasks (all completed)
+ PinotHelixTaskResourceManager.TaskCount taskCount = new
PinotHelixTaskResourceManager.TaskCount();
+ taskCount.addTaskState(TaskPartitionState.COMPLETED);
+ taskCount.addTaskState(TaskPartitionState.COMPLETED);
+ Map<String, PinotHelixTaskResourceManager.TaskCount> taskCounts = new
HashMap<>();
+ taskCounts.put(taskName, taskCount);
+ when(spyMgr.getTaskCounts(taskType)).thenReturn(taskCounts);
+
+ PinotHelixTaskResourceManager.TaskSummaryResponse response =
spyMgr.getTasksSummary(null);
+
+ assertNotNull(response);
+ assertEquals(response.getTotalRunningTasks(), 0);
+ assertEquals(response.getTotalWaitingTasks(), 0);
+ assertTrue(response.getTaskBreakdown().isEmpty());
+ }
+
+ @Test
+ public void testGetTasksSummaryWithSingleTenant() {
+ TaskDriver taskDriver = mock(TaskDriver.class);
+ PinotHelixResourceManager helixResourceManager =
mock(PinotHelixResourceManager.class);
+ PinotHelixTaskResourceManager mgr = new
PinotHelixTaskResourceManager(helixResourceManager, taskDriver);
+
+ String taskType = "TestTask";
+ String taskName = "Task_TestTask_12345";
+ String tableName = "testTable_OFFLINE";
+ String tenant = "defaultTenant";
+
+ PinotHelixTaskResourceManager spyMgr = Mockito.spy(mgr);
+ when(spyMgr.getTaskTypes()).thenReturn(Collections.singleton(taskType));
+
+ // Task with running and waiting tasks
+ PinotHelixTaskResourceManager.TaskCount taskCount = new
PinotHelixTaskResourceManager.TaskCount();
+ taskCount.addTaskState(TaskPartitionState.RUNNING);
+ taskCount.addTaskState(TaskPartitionState.RUNNING);
+ taskCount.addTaskState(null); // null state counts as waiting
Review 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.
```suggestion
// In TaskCount.addTaskState, a null state is interpreted as a waiting
task.
// See PinotHelixTaskResourceManager.TaskCount#addTaskState for
implementation details.
taskCount.addTaskState(null);
```
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotHelixTaskResourceManager.java:
##########
@@ -83,6 +84,7 @@ public class PinotHelixTaskResourceManager {
private static final String TASK_QUEUE_PREFIX = "TaskQueue" +
TASK_NAME_SEPARATOR;
private static final String TASK_PREFIX = "Task" + TASK_NAME_SEPARATOR;
private static final String UNKNOWN_TABLE_NAME = "unknown";
+ private static final String UNKNOWN_TENANT_NAME = "unknown";
Review 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.
```suggestion
private static final String UNKNOWN_TABLE_NAME = CommonConstants.UNKNOWN;
```
--
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]
