devmadhuu commented on code in PR #11066:
URL: https://github.com/apache/ozone/pull/11066#discussion_r3860214624
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/datanode/ListInfoSubcommand.java:
##########
@@ -204,14 +205,15 @@ private void printDatanodeInfo(BasicDatanodeInfo dn) {
.append(System.lineSeparator());
} else {
relatedPipelineNum = relatedPipelines.size();
- relatedPipelines.forEach(
- p -> pipelineListInfo.append(p.getId().getId().toString())
- .append('/').append(p.getReplicationConfig().toString())
- .append('/').append(p.getType().toString())
-
.append('/').append(p.getPipelineState().toString()).append('/')
- .append(datanode.getID().equals(p.getLeaderId()) ?
- "Leader" : "Follower")
- .append(System.lineSeparator()));
+ relatedPipelines.forEach(p -> {
Review Comment:
Any reason the code is changed from chained style to lambda block style ?
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/datanode/ListInfoSubcommand.java:
##########
@@ -243,4 +245,8 @@ private void printDatanodeInfo(BasicDatanodeInfo dn) {
System.out.printf("Percentage Used : %.2f%%%n%n", dn.getPercentUsed());
}
}
+
+ private static String formatSupportedStorageTier(StorageTier tier) {
+ return tier == null ? "[]" : "[" + tier.getTierName() + "]";
Review Comment:
from user perspective, both empty and "EMPTY" are same, so why we should
have two representations for same thing ? . Treat null and `EMPTY` identically
in `formatSupportedStorageTier`. Either both become [] (preferred — matches "no
tier set") or both become `[EMPTY] `(matches enum name).
@xichen01 - do you agree ? Also the null tier will anyway not exist in real
path.
##########
hadoop-ozone/cli-admin/src/test/java/org/apache/hadoop/hdds/scm/cli/datanode/TestListInfoSubcommand.java:
##########
@@ -459,6 +466,54 @@ private void validateOrderingFromTextOutput(String output,
String orderDirection
}
}
+ @ParameterizedTest
+ @EnumSource(value = StorageTier.class, names = {"EMPTY"}, mode =
EnumSource.Mode.EXCLUDE)
+ public void testRelatedPipelineShowsStorageTier(StorageTier tier) throws
Exception {
+ List<HddsProtos.Node> nodes = getNodeDetails();
+ HddsProtos.Node node = nodes.get(0);
+
+ ScmClient scmClient = mock(ScmClient.class);
+ when(scmClient.queryNode(any(), any(), any(), any())).thenReturn(nodes);
+ when(scmClient.listPipelines()).thenReturn(
+ Collections.singletonList(pipelineContaining(node, tier)));
+
+ CommandLine c = new CommandLine(cmd);
+ c.parseArgs();
+ cmd.execute(scmClient);
+
+ String output = outContent.toString(DEFAULT_ENCODING);
+ assertThat(output).contains("/[" + tier.getTierName() + "]");
Review Comment:
I think better to assert with full pattern:
`assertThat(output).containsPattern("/[^/]+/[^/]+/[^/]+/[^/]+/(Leader|Follower)/\\["
+ tier.getTierName() + "\\]");`
##########
hadoop-ozone/cli-admin/src/test/java/org/apache/hadoop/hdds/scm/cli/datanode/TestListInfoSubcommand.java:
##########
@@ -459,6 +466,54 @@ private void validateOrderingFromTextOutput(String output,
String orderDirection
}
}
+ @ParameterizedTest
+ @EnumSource(value = StorageTier.class, names = {"EMPTY"}, mode =
EnumSource.Mode.EXCLUDE)
+ public void testRelatedPipelineShowsStorageTier(StorageTier tier) throws
Exception {
+ List<HddsProtos.Node> nodes = getNodeDetails();
+ HddsProtos.Node node = nodes.get(0);
Review Comment:
Should have a test which can test with both leader and follower labels
within same pipeline.
##########
hadoop-ozone/cli-admin/src/test/java/org/apache/hadoop/hdds/scm/cli/datanode/TestListInfoSubcommand.java:
##########
@@ -459,6 +466,54 @@ private void validateOrderingFromTextOutput(String output,
String orderDirection
}
}
+ @ParameterizedTest
+ @EnumSource(value = StorageTier.class, names = {"EMPTY"}, mode =
EnumSource.Mode.EXCLUDE)
+ public void testRelatedPipelineShowsStorageTier(StorageTier tier) throws
Exception {
+ List<HddsProtos.Node> nodes = getNodeDetails();
+ HddsProtos.Node node = nodes.get(0);
+
+ ScmClient scmClient = mock(ScmClient.class);
+ when(scmClient.queryNode(any(), any(), any(), any())).thenReturn(nodes);
+ when(scmClient.listPipelines()).thenReturn(
+ Collections.singletonList(pipelineContaining(node, tier)));
+
+ CommandLine c = new CommandLine(cmd);
+ c.parseArgs();
+ cmd.execute(scmClient);
+
+ String output = outContent.toString(DEFAULT_ENCODING);
+ assertThat(output).contains("/[" + tier.getTierName() + "]");
+ }
+
+ @Test
+ public void testRelatedPipelineOmitsTierWhenNull() throws Exception {
Review Comment:
IMO, why we should test something when in real path, there won't be a
pipeline with null tier ?
@xichen01 - your opinion ?
--
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]
