Copilot commented on code in PR #18755:
URL: https://github.com/apache/pinot/pull/18755#discussion_r3408693559
##########
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:
In the comma-separated string fallback, the split values are returned
without trimming and may include empty entries (e.g. "a, b," -> ["a", " b",
""]). This makes the parsed result inconsistent with the JSON-array path and
can propagate whitespace/empty elements to callers.
##########
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:
validateUpdateInstanceTags() builds the request body by splitting on commas,
but it does not drop empty tokens. Inputs with trailing commas or
whitespace-only entries will generate requests with empty instance names or
empty tag strings, which is unlikely to be valid for the controller validation
endpoint.
##########
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:
These assertions expect String values ("COMPLETED"), but
TaskAdminClient.getTaskStates*() is typed as Map<String, TaskState>. Aligning
the test with the declared API will catch incorrect deserialization (e.g.,
returning Strings instead of TaskState enums).
##########
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:
getTaskStatesByTable() is declared to return Map<String, TaskState>, but
convertValue(response, Map.class) will deserialize enum values as Strings (or
Maps), which can lead to ClassCastException for callers expecting TaskState.
Since the controller returns Map<String, TaskState>, this should deserialize
directly to TaskState.
##########
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:
getTaskStates() is declared to return Map<String, TaskState>, but
convertValue(response, Map.class) will deserialize enum values as Strings, so
callers can hit ClassCastException when reading values as TaskState. Use a
TypeReference so Jackson converts the values to TaskState enums.
--
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]
