xiangfu0 commented on code in PR #18755:
URL: https://github.com/apache/pinot/pull/18755#discussion_r3408701985
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/admin/PinotAdminTransport.java:
##########
@@ -443,24 +443,42 @@ public List<String> parseStringArray(JsonNode response,
String fieldName)
if (arrayNode == null) {
throw new PinotAdminException("Response missing '" + fieldName + "'
field");
}
+ return parseStringArrayNode(arrayNode);
+ }
+ /**
+ * Parses a JSON node that is itself a string array (or a comma-separated
textual value) into a list of strings.
+ *
+ * <p>This is the single implementation shared by {@link
#parseStringArray(JsonNode, String)} (which first extracts a
+ * named field) and the admin clients that consume controller endpoints
returning a bare JSON array (for example
+ * {@code GET /schemas} and {@code DELETE /tables/{tableName}/rebalance}).
+ *
+ * @param arrayNode the node expected to be a JSON array (or a
comma-separated string)
+ * @return the parsed list of strings
+ * @throws PinotAdminException if {@code arrayNode} is {@code null} or is
neither an array nor a string
+ */
+ static List<String> parseStringArrayNode(JsonNode arrayNode)
+ throws PinotAdminException {
+ if (arrayNode == null || arrayNode.isNull()) {
+ throw new PinotAdminException("Expected a JSON array but got a null
node");
+ }
if (arrayNode.isArray()) {
// Handle JSON array format
- List<String> result = new ArrayList<>();
+ List<String> result = new ArrayList<>(arrayNode.size());
for (JsonNode element : arrayNode) {
result.add(element.asText());
}
return result;
- } else if (arrayNode.isTextual()) {
+ }
+ if (arrayNode.isTextual()) {
// Handle comma-separated string format for backward compatibility
String text = arrayNode.asText().trim();
if (text.isEmpty()) {
return Collections.emptyList();
Review Comment:
Fixed — the CSV fallback in `parseStringArrayNode` now trims each token and
drops empty entries, matching the JSON-array path.
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/admin/InstanceAdminClient.java:
##########
@@ -248,19 +250,31 @@ public String validateDropInstances(String instanceNames)
/**
* Validates whether it's safe to update the tags of the given instances.
*
+ * <p>The controller endpoint reads a {@code List<InstanceTagUpdateRequest>}
from the request body (not query
+ * params). This method builds one request per instance name, all sharing
the given tags. Use
+ * {@link #validateInstanceTagUpdates(String)} when you need to supply
per-instance tags.
+ *
* @param instanceNames Comma-separated list of instance names to validate
- * @param newTags New tags to assign
+ * @param newTags Comma-separated list of tags to assign to every listed
instance
* @return Validation response as JSON string
* @throws PinotAdminException If the request fails
*/
public String validateUpdateInstanceTags(String instanceNames, String
newTags)
throws PinotAdminException {
- Map<String, String> queryParams = new HashMap<>();
- queryParams.put("instanceNames", instanceNames);
- queryParams.put("newTags", newTags);
+ List<String> tags = new ArrayList<>();
+ for (String tag : newTags.split(",")) {
+ tags.add(tag.trim());
+ }
+ List<Map<String, Object>> requestBody = new ArrayList<>();
Review Comment:
Fixed — empty/whitespace-only instance names and tags are now skipped before
building the request body.
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/admin/TaskAdminClient.java:
##########
@@ -106,7 +106,7 @@ public Map<String, TaskState> getTaskStatesByTable(String
taskType, String table
JsonNode response =
_transport.executeGet(_controllerAddress, "/tasks/" + taskType + "/" +
tableNameWithType + "/state",
null, _headers);
- return
PinotAdminTransport.getObjectMapper().convertValue(response.get("taskStates"),
Map.class);
+ return PinotAdminTransport.getObjectMapper().convertValue(response,
Map.class);
}
Review Comment:
Fixed — now deserializes with `TypeReference<Map<String, TaskState>>` so the
values are real `TaskState` enums.
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/admin/TaskAdminClient.java:
##########
@@ -257,7 +260,7 @@ public String getTaskDebugInfo(String taskName, int
verbosity, @Nullable String
public Map<String, TaskState> getTaskStates(String taskType)
throws PinotAdminException {
JsonNode response = _transport.executeGet(_controllerAddress, "/tasks/" +
taskType + "/taskstates", null, _headers);
- return
PinotAdminTransport.getObjectMapper().convertValue(response.get("taskStates"),
Map.class);
+ return PinotAdminTransport.getObjectMapper().convertValue(response,
Map.class);
}
Review Comment:
Fixed — now uses `TypeReference<Map<String, TaskState>>` so Jackson converts
the values to `TaskState` enums.
##########
pinot-clients/pinot-java-client/src/test/java/org/apache/pinot/client/admin/PinotAdminClientTest.java:
##########
@@ -432,4 +435,201 @@ public void
testRebalanceAndQueryClientsUseDedicatedEndpoints()
verify(_mockTransport).executeDelete(eq(CONTROLLER_ADDRESS),
eq("/clientQuery/client-query-1"), isNull(),
eq(HEADERS));
}
+
+ @Test
+ public void testListLiveInstancesUsesInstancesField()
+ throws Exception {
+ // GET /liveinstances returns an Instances wrapper serialized as
{"instances": [...]}.
+ JsonNode mockResponse = new ObjectMapper().readTree("{\"instances\":
[\"Server_1\", \"Broker_1\"]}");
+ lenient().when(_mockTransport.executeGet(anyString(), anyString(), any(),
any())).thenReturn(mockResponse);
+
+ List<String> instances =
_adminClient.getInstanceClient().listLiveInstances();
+
+ assertEquals(instances, List.of("Server_1", "Broker_1"));
+ verify(_mockTransport).executeGet(eq(CONTROLLER_ADDRESS),
eq("/liveinstances"), isNull(), eq(HEADERS));
+ }
+
+ @Test
+ public void testValidateUpdateInstanceTagsSendsRequestBody()
+ throws Exception {
+ JsonNode mockResponse = new ObjectMapper().readTree("[]");
+ lenient().when(_mockTransport.executePost(anyString(), anyString(), any(),
any(), any()))
+ .thenReturn(mockResponse);
+
+
_adminClient.getInstanceClient().validateUpdateInstanceTags("Server_1,Server_2",
"tag1,tag2");
+
+ // Controller reads a List<InstanceTagUpdateRequest> from the body; no
query params are used.
+ List<Map<String, Object>> expectedBody = List.of(
+ Map.of("instanceName", "Server_1", "newTags", List.of("tag1", "tag2")),
+ Map.of("instanceName", "Server_2", "newTags", List.of("tag1",
"tag2")));
+ verify(_mockTransport).executePost(eq(CONTROLLER_ADDRESS),
eq("/instances/updateTags/validate"), eq(expectedBody),
+ isNull(), eq(HEADERS));
+ }
+
+ @Test
+ public void testCancelRebalanceParsesBareArray()
+ throws Exception {
+ // DELETE /tables/{tableName}/rebalance returns a bare JSON array of
cancelled job IDs.
+ JsonNode mockResponse = new ObjectMapper().readTree("[\"job-1\",
\"job-2\"]");
+ lenient().when(_mockTransport.executeDelete(anyString(), anyString(),
any(), any())).thenReturn(mockResponse);
+
+ List<String> jobIds =
_adminClient.getTableClient().cancelRebalance("tbl1_OFFLINE");
+
+ assertEquals(jobIds, List.of("job-1", "job-2"));
+ verify(_mockTransport).executeDelete(eq(CONTROLLER_ADDRESS),
eq("/tables/tbl1_OFFLINE/rebalance"), isNull(),
+ eq(HEADERS));
+ }
+
+ @Test
+ public void testRebalanceTableMapsDowntimeAndMinAvailableReplicas()
+ throws Exception {
+ JsonNode mockResponse = new ObjectMapper().readTree("{\"status\":\"OK\"}");
+ lenient().when(_mockTransport.executePost(anyString(), anyString(), any(),
any(), any()))
+ .thenReturn(mockResponse);
+
+ _adminClient.getTableClient().rebalanceTable("tbl1_OFFLINE", true, 2);
+
+ verify(_mockTransport).executePost(eq(CONTROLLER_ADDRESS),
eq("/tables/tbl1_OFFLINE/rebalance"), isNull(),
+ eq(Map.of("downtime", "false", "minAvailableReplicas", "2")),
eq(HEADERS));
+ }
+
+ @Test
+ public void testSelectSegmentsUsesStartEndTimestampParams()
+ throws Exception {
+ JsonNode mockResponse = new ObjectMapper().readTree("[]");
+ lenient().when(_mockTransport.executeGet(anyString(), anyString(), any(),
any())).thenReturn(mockResponse);
+
+ _adminClient.getSegmentClient().selectSegments("tbl1", "OFFLINE", 100L,
200L, true);
+
+ verify(_mockTransport).executeGet(eq(CONTROLLER_ADDRESS),
eq("/segments/tbl1/select"),
+ eq(Map.of("startTimestamp", "100", "endTimestamp", "200",
"excludeReplacedSegments", "true", "type",
+ "OFFLINE")), eq(HEADERS));
+ }
+
+ @Test
+ public void testGetZookeeperMetadataParsesBareMap()
+ throws Exception {
+ JsonNode mockResponse = new ObjectMapper()
+
.readTree("{\"segment_1\":{\"segment.crc\":\"123\"},\"segment_2\":{\"segment.crc\":\"456\"}}");
+ lenient().when(_mockTransport.executeGet(anyString(), anyString(), any(),
any())).thenReturn(mockResponse);
+
+ Map<String, Map<String, String>> zkMetadata =
_adminClient.getSegmentClient().getZookeeperMetadata("tbl1_OFFLINE");
+
+ assertEquals(zkMetadata.size(), 2);
+ assertEquals(zkMetadata.get("segment_1").get("segment.crc"), "123");
+ verify(_mockTransport).executeGet(eq(CONTROLLER_ADDRESS),
eq("/segments/tbl1_OFFLINE/zkmetadata"), isNull(),
+ eq(HEADERS));
+ }
+
+ @Test
+ public void testTaskEndpointsParseBareValues()
+ throws Exception {
+ ObjectMapper mapper = new ObjectMapper();
+ // Every task endpoint resumes a bare value (array/string/int/map), not a
wrapper object.
+ lenient().when(_mockTransport.executeGet(eq(CONTROLLER_ADDRESS),
eq("/tasks/tasktypes"), any(), any()))
+ .thenReturn(mapper.readTree("[\"TaskA\", \"TaskB\"]"));
+ lenient().when(_mockTransport.executeGet(eq(CONTROLLER_ADDRESS),
eq("/tasks/TaskA/state"), any(), any()))
+ .thenReturn(mapper.readTree("\"IN_PROGRESS\""));
+ lenient().when(_mockTransport.executeGet(eq(CONTROLLER_ADDRESS),
eq("/tasks/TaskA/tasks"), any(), any()))
+ .thenReturn(mapper.readTree("[\"Task_TaskA_1\"]"));
+ lenient().when(_mockTransport.executeGet(eq(CONTROLLER_ADDRESS),
eq("/tasks/TaskA/tasks/count"), any(), any()))
+ .thenReturn(mapper.readTree("5"));
+ lenient().when(_mockTransport.executeGet(eq(CONTROLLER_ADDRESS),
eq("/tasks/TaskA/taskstates"), any(), any()))
+ .thenReturn(mapper.readTree("{\"Task_TaskA_1\":\"COMPLETED\"}"));
+ lenient().when(_mockTransport.executeGet(eq(CONTROLLER_ADDRESS),
eq("/tasks/TaskA/tbl1_OFFLINE/state"), any(),
+
any())).thenReturn(mapper.readTree("{\"Task_TaskA_1\":\"COMPLETED\"}"));
+ lenient().when(_mockTransport.executeGet(eq(CONTROLLER_ADDRESS),
eq("/tasks/subtask/Task_TaskA_1/state"), any(),
+
any())).thenReturn(mapper.readTree("{\"Task_TaskA_1_0\":\"RUNNING\"}"));
+
+ TaskAdminClient taskClient = _adminClient.getTaskClient();
+ assertEquals(taskClient.listTaskTypes(), Set.of("TaskA", "TaskB"));
+ assertEquals(taskClient.getTaskQueueState("TaskA"), TaskState.IN_PROGRESS);
+ assertEquals(taskClient.getTasks("TaskA"), Set.of("Task_TaskA_1"));
+ assertEquals(taskClient.getTasksCount("TaskA"), 5);
+ assertEquals(taskClient.getTaskStates("TaskA"), Map.of("Task_TaskA_1",
"COMPLETED"));
+ assertEquals(taskClient.getTaskStatesByTable("TaskA", "tbl1_OFFLINE"),
Map.of("Task_TaskA_1", "COMPLETED"));
Review Comment:
Fixed — the assertions now expect `TaskState.COMPLETED` enums, aligning the
test with the declared `Map<String, TaskState>` API.
--
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]
