You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2020/12/10 16:25:58 UTC
[GitHub] [cassandra] swolfenhaut opened a new pull request #845: CASSANDRA-16283: fix output on nodetool -r
swolfenhaut opened a new pull request #845:
URL: https://github.com/apache/cassandra/pull/845
----------------------------------------------------------------
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
[GitHub] [cassandra] bereng commented on a change in pull request #845: CASSANDRA-16283: fix output on nodetool -r
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #845:
URL: https://github.com/apache/cassandra/pull/845#discussion_r555508245
##########
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:
+1 given it's an easy thing to do
----------------------------------------------------------------
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
[GitHub] [cassandra] bereng commented on a change in pull request #845: CASSANDRA-16283: fix output on nodetool -r
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #845:
URL: https://github.com/apache/cassandra/pull/845#discussion_r555507873
##########
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:
+1
----------------------------------------------------------------
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
[GitHub] [cassandra] aholmberg commented on a change in pull request #845: CASSANDRA-16283: fix output on nodetool -r
Posted by 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
[GitHub] [cassandra] bereng commented on a change in pull request #845: CASSANDRA-16283: fix output on nodetool -r
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #845:
URL: https://github.com/apache/cassandra/pull/845#discussion_r555508057
##########
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:
+1 Either we resolve this here or we open a new ticket if you don't want to drag this one longer
----------------------------------------------------------------
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
[GitHub] [cassandra] bereng commented on a change in pull request #845: CASSANDRA-16283: fix output on nodetool -r
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #845:
URL: https://github.com/apache/cassandra/pull/845#discussion_r555510028
##########
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"));
+ tool.assertCleanStdErr();
Review comment:
These 2 can be replaced by `assertOnCleanExit()`
----------------------------------------------------------------
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
[GitHub] [cassandra] smiklosovic closed pull request #845: CASSANDRA-16283: fix output on nodetool -r
Posted by GitBox <gi...@apache.org>.
smiklosovic closed pull request #845:
URL: https://github.com/apache/cassandra/pull/845
--
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: pr-unsubscribe@cassandra.apache.org
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