[GitHub] [ozone] adoroszlai commented on a change in pull request #2406: HDDS-5402 Support list node based on NodeOperationalState and NodeState options in printTopology CLI

[GitBox <gi...@apache.org>]

adoroszlai commented on a change in pull request #2406:
URL: https://github.com/apache/ozone/pull/2406#discussion_r669568768



##########
File path: hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/TopologySubcommand.java
##########
@@ -139,6 +151,11 @@ private String getAdditionNodeOutput(HddsProtos.Node node) {
   // Format "ipAddress(hostName):PortName1=PortValue1    OperationalState
   //     networkLocation
   private void printNodesWithLocation(Collection<HddsProtos.Node> nodes) {
+    if (nodeOperationalState != null) {
+      nodes = nodes.stream().filter(
+          info -> info.getNodeOperationalStates(0).toString()
+              .equals(nodeOperationalState)).collect(Collectors.toList());
+    }

Review comment:
       Nit: can you please avoid code duplication by extracting the code block to a method.  It would also make sense to invoke it once from `execute()`, not in each `print...()`.

##########
File path: hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/TopologySubcommand.java
##########
@@ -68,6 +69,13 @@
       description = "Print Topology with full node infos")
   private boolean fullInfo;
 
+  @CommandLine.Option(names = {"-n", "--nodeOperationalState"},
+      description = "Show info by datanode NodeOperationalState" +

Review comment:
       I think "show ... by" is ambiguous here, could mean grouping or filtering.  I suggest:
   
   ```suggestion
         description = "Only show datanodes in a specific operational state " +
   ```

##########
File path: hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/TopologySubcommand.java
##########
@@ -68,6 +69,13 @@
       description = "Print Topology with full node infos")
   private boolean fullInfo;
 
+  @CommandLine.Option(names = {"-n", "--nodeOperationalState"},

Review comment:
       Options are usually preferred in this form: `--operational-state`.  The camelCase form is consistent with the command name, though.




-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org