[GitHub] [cassandra] aholmberg commented on a change in pull request #845: CASSANDRA-16283: fix output on nodetool -r

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

aholmberg commented on a change in pull request #845:
URL: https://github.com/apache/cassandra/pull/845#discussion_r552855324



##########
File path: src/java/org/apache/cassandra/tools/nodetool/Status.java
##########
@@ -149,28 +150,33 @@ private void addNodesHeader(boolean hasEffectiveOwns, TableBuilder tableBuilder)
     private void addNode(String endpoint, Float owns, String epDns, String token, int size, boolean hasEffectiveOwns,

Review comment:
       What would you think of just refactoring this to accept the HostStatWithPort? Then we have access to `ipOrDns()` as well as the `endpoint`, and we don't need to resolve again.
   https://github.com/apache/cassandra/blob/401e933b7395892bf0356f88308f64b94be84601/src/java/org/apache/cassandra/tools/nodetool/HostStat.java#L24)

##########
File path: test/unit/org/apache/cassandra/tools/nodetool/stats/NodetoolTableStatsTest.java
##########
@@ -237,4 +237,13 @@ public void testTopArg()
         tool.assertCleanStdErr();
         assertEquals(1, tool.getExitCode());
     }
+
+    @Test
+    public void testStatusArg()
+    {
+        ToolResult tool = ToolRunner.invokeNodetool("status", "-r");

Review comment:
       I think this bug goes a bit further than just the resolved host lookup logic. It seems like the tool is never showing anything definitive for the "Owns (effective)" column. I'll bubble this up on the ticket.

##########
File path: test/unit/org/apache/cassandra/tools/nodetool/stats/NodetoolTableStatsTest.java
##########
@@ -237,4 +237,13 @@ public void testTopArg()
         tool.assertCleanStdErr();
         assertEquals(1, tool.getExitCode());
     }
+
+    @Test
+    public void testStatusArg()
+    {
+        ToolResult tool = ToolRunner.invokeNodetool("status", "-r");
+        assertThat(tool.getStdout(), CoreMatchers.containsString("UN"));

Review comment:
       I wonder if this test should go a bit further and validate that the IP actually resolves. Can we count on `127.0.0.1` to always resolve to `localhost` in these tests?

##########
File path: test/unit/org/apache/cassandra/tools/nodetool/stats/NodetoolTableStatsTest.java
##########
@@ -237,4 +237,13 @@ public void testTopArg()
         tool.assertCleanStdErr();
         assertEquals(1, tool.getExitCode());
     }
+
+    @Test
+    public void testStatusArg()

Review comment:
       Thanks for adding a test. I'm wondering if this should be a new package in nodetool tests since we're testing `status` and not `stats`.




----------------------------------------------------------------
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.

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



---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org