From 9e730e481021f31d7a72746540b51866cc4d7b9c Mon Sep 17 00:00:00 2001 From: Gargi Jaiswal Date: Wed, 3 Sep 2025 16:33:00 +0530 Subject: [PATCH 1/3] use getValidatedIDs to validate and convert container IDs by CLI --- .../scm/cli/container/InfoSubcommand.java | 28 ++++++------------- .../main/smoketest/admincli/container.robot | 5 ++++ 2 files changed, 14 insertions(+), 19 deletions(-) diff --git a/hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/container/InfoSubcommand.java b/hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/container/InfoSubcommand.java index 34a5c48656f2..82acb0a8f206 100644 --- a/hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/container/InfoSubcommand.java +++ b/hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/container/InfoSubcommand.java @@ -65,30 +65,23 @@ public class InfoSubcommand extends ScmSubcommand { @Override public void execute(ScmClient scmClient) throws IOException { + // validate all container IDs and fail fast + List containerIDs = containerList.getValidatedIDs(); + boolean first = true; - multiContainer = containerList.size() > 1; + multiContainer = containerIDs.size() > 1; printHeader(); - // TODO HDDS-13592: Use ContainerIDParameters#getValidatedIDs to automatically handle type conversion and fail fast. - for (String id : containerList) { - printOutput(scmClient, id, first); + for (Long containerID : containerIDs) { + if (!first) { + printBreak(); + } + printDetails(scmClient, containerID, first); first = false; } printFooter(); } - private void printOutput(ScmClient scmClient, String id, boolean first) - throws IOException { - long containerID; - try { - containerID = Long.parseLong(id); - } catch (NumberFormatException e) { - printError("Invalid container ID: " + id); - return; - } - printDetails(scmClient, containerID, first); - } - private void printHeader() { if (json && multiContainer) { System.out.println("["); @@ -131,9 +124,6 @@ private void printDetails(ScmClient scmClient, long containerID, printError("Unable to retrieve the replica details: " + e.getMessage()); } - if (!first) { - printBreak(); - } if (json) { if (!container.getPipeline().isEmpty()) { ContainerWithPipelineAndReplicas wrapper = diff --git a/hadoop-ozone/dist/src/main/smoketest/admincli/container.robot b/hadoop-ozone/dist/src/main/smoketest/admincli/container.robot index e9450c9de595..982ebf06bd20 100644 --- a/hadoop-ozone/dist/src/main/smoketest/admincli/container.robot +++ b/hadoop-ozone/dist/src/main/smoketest/admincli/container.robot @@ -93,6 +93,11 @@ Container info Should contain ${output} Pipeline id Should contain ${output} Datanodes +Container info should fail with invalid container ID + ${output} = Execute ozone admin container info "${CONTAINER}" -2 0.5 abc + Should contain ${output} Container IDs must be positive integers + Should contain ${output} Invalid container ID: -2 0.5 abc + Verbose container info ${output} = Execute ozone admin --verbose container info "${CONTAINER}" Should contain ${output} Pipeline Info From 49dcd9e237bde015db7852fd70a229e6354eb405 Mon Sep 17 00:00:00 2001 From: Gargi Jaiswal Date: Thu, 4 Sep 2025 09:00:28 +0530 Subject: [PATCH 2/3] fixed robot test # Conflicts: # hadoop-ozone/cli-admin/src/test/java/org/apache/hadoop/hdds/scm/cli/container/TestInfoSubCommand.java --- .../scm/cli/container/TestInfoSubCommand.java | 55 ++++++++++++++----- .../main/smoketest/admincli/container.robot | 6 +- 2 files changed, 45 insertions(+), 16 deletions(-) diff --git a/hadoop-ozone/cli-admin/src/test/java/org/apache/hadoop/hdds/scm/cli/container/TestInfoSubCommand.java b/hadoop-ozone/cli-admin/src/test/java/org/apache/hadoop/hdds/scm/cli/container/TestInfoSubCommand.java index a0bee39530d2..8e3d2abe3f1b 100644 --- a/hadoop-ozone/cli-admin/src/test/java/org/apache/hadoop/hdds/scm/cli/container/TestInfoSubCommand.java +++ b/hadoop-ozone/cli-admin/src/test/java/org/apache/hadoop/hdds/scm/cli/container/TestInfoSubCommand.java @@ -119,14 +119,22 @@ public void testMultipleContainersCanBePassed() throws Exception { when(scmClient.getContainerReplicas(anyLong())).thenReturn(getReplicas(true)); cmd = new InfoSubcommand(); CommandLine c = new CommandLine(cmd); - c.parseArgs("1", "123", "456", "invalid", "789"); + c.parseArgs("1", "123", "456", "789"); cmd.execute(scmClient); validateMultiOutput(); } + @Test + public void testMultipleInvalidContainerIdFails() throws Exception { + cmd = new InfoSubcommand(); + CommandLine c = new CommandLine(cmd); + c.parseArgs("1", "invalid", "-2", "0.5"); + validateInvalidContainerIDOutput(); + } + @Test public void testContainersCanBeReadFromStdin() throws IOException { - String input = "1\n123\n456\ninvalid\n789\n"; + String input = "1\n123\n456\n789\n"; ByteArrayInputStream inContent = new ByteArrayInputStream(input.getBytes(DEFAULT_ENCODING)); System.setIn(inContent); cmd = new InfoSubcommand(); @@ -137,23 +145,38 @@ public void testContainersCanBeReadFromStdin() throws IOException { validateMultiOutput(); } + @Test + public void testInvalidContainerIdFromStdinFails() throws Exception { + String input = "1\ninvalid\n-2\n0.5\n"; + ByteArrayInputStream inContent = new ByteArrayInputStream(input.getBytes(DEFAULT_ENCODING)); + System.setIn(inContent); + cmd = new InfoSubcommand(); + CommandLine c = new CommandLine(cmd); + c.parseArgs("-"); + validateInvalidContainerIDOutput(); + } + private void validateMultiOutput() throws UnsupportedEncodingException { // Ensure we have a log line for each containerID List replica = Arrays.stream(outContent.toString(DEFAULT_ENCODING).split("\n")) .filter(m -> m.matches("(?s)^Container id: (1|123|456|789).*")) .collect(Collectors.toList()); assertEquals(4, replica.size()); + } - Pattern p = Pattern.compile( - "^Invalid\\scontainer\\sID:\\sinvalid.*", Pattern.MULTILINE); - Matcher m = p.matcher(errContent.toString(DEFAULT_ENCODING)); - assertTrue(m.find()); + private void validateInvalidContainerIDOutput() throws Exception { + CommandLine.ParameterException ex = assertThrows( + CommandLine.ParameterException.class, () -> cmd.execute(scmClient)); + + assertThat(ex.getMessage()) + .isEqualTo("Container IDs must be positive integers. Invalid container IDs: invalid -2 0.5"); + assertThat(outContent.toString(DEFAULT_ENCODING)).isEmpty(); } @Test public void testContainersCanBeReadFromStdinJson() throws IOException { - String input = "1\n123\n456\ninvalid\n789\n"; + String input = "1\n123\n456\n789\n"; ByteArrayInputStream inContent = new ByteArrayInputStream(input.getBytes(DEFAULT_ENCODING)); System.setIn(inContent); cmd = new InfoSubcommand(); @@ -164,12 +187,23 @@ public void testContainersCanBeReadFromStdinJson() validateJsonMultiOutput(); } + @Test + public void testInvalidContainerIdFromStdinJsonFails() throws Exception { + String input = "1\ninvalid\n-2\n0.5\n"; + ByteArrayInputStream inContent = new ByteArrayInputStream(input.getBytes(DEFAULT_ENCODING)); + System.setIn(inContent); + cmd = new InfoSubcommand(); + CommandLine c = new CommandLine(cmd); + c.parseArgs("-", "--json"); + validateInvalidContainerIDOutput(); + } + @Test public void testMultipleContainersCanBePassedJson() throws Exception { when(scmClient.getContainerReplicas(anyLong())).thenReturn(getReplicas(true)); cmd = new InfoSubcommand(); CommandLine c = new CommandLine(cmd); - c.parseArgs("1", "123", "456", "invalid", "789", "--json"); + c.parseArgs("1", "123", "456", "789", "--json"); cmd.execute(scmClient); validateJsonMultiOutput(); @@ -181,11 +215,6 @@ private void validateJsonMultiOutput() throws UnsupportedEncodingException { .filter(m -> m.matches("(?s)^.*\"containerInfo\".*")) .collect(Collectors.toList()); assertEquals(4, replica.size()); - - Pattern p = Pattern.compile( - "^Invalid\\scontainer\\sID:\\sinvalid.*", Pattern.MULTILINE); - Matcher m = p.matcher(errContent.toString(DEFAULT_ENCODING)); - assertTrue(m.find()); } private void testReplicaIncludedInOutput(boolean includeIndex) diff --git a/hadoop-ozone/dist/src/main/smoketest/admincli/container.robot b/hadoop-ozone/dist/src/main/smoketest/admincli/container.robot index 982ebf06bd20..f0c11b0881cf 100644 --- a/hadoop-ozone/dist/src/main/smoketest/admincli/container.robot +++ b/hadoop-ozone/dist/src/main/smoketest/admincli/container.robot @@ -94,9 +94,9 @@ Container info Should contain ${output} Datanodes Container info should fail with invalid container ID - ${output} = Execute ozone admin container info "${CONTAINER}" -2 0.5 abc - Should contain ${output} Container IDs must be positive integers - Should contain ${output} Invalid container ID: -2 0.5 abc + ${output} = Execute And Ignore Error ozone admin container info "${CONTAINER}" -2 0.5 abc + Should contain ${output} Container IDs must be positive integers. + Should contain ${output} Invalid container IDs: -2 0.5 abc Verbose container info ${output} = Execute ozone admin --verbose container info "${CONTAINER}" From 49a7d883432e00eba5279484fbd91715e6f1d5f5 Mon Sep 17 00:00:00 2001 From: Gargi Jaiswal Date: Mon, 15 Sep 2025 09:59:53 +0530 Subject: [PATCH 3/3] removed first param --- .../apache/hadoop/hdds/scm/cli/container/InfoSubcommand.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/container/InfoSubcommand.java b/hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/container/InfoSubcommand.java index 82acb0a8f206..c48e6251eb5e 100644 --- a/hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/container/InfoSubcommand.java +++ b/hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/container/InfoSubcommand.java @@ -76,7 +76,7 @@ public void execute(ScmClient scmClient) throws IOException { if (!first) { printBreak(); } - printDetails(scmClient, containerID, first); + printDetails(scmClient, containerID); first = false; } printFooter(); @@ -106,8 +106,7 @@ private void printBreak() { } } - private void printDetails(ScmClient scmClient, long containerID, - boolean first) throws IOException { + private void printDetails(ScmClient scmClient, long containerID) throws IOException { final ContainerWithPipeline container; try { container = scmClient.getContainerWithPipeline(containerID);