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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
