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 2022/10/13 13:50:10 UTC
[GitHub] [cassandra] maxim-chn opened a new pull request, #1910: Resolve IP option for Nodetool gossiping command
maxim-chn opened a new pull request, #1910:
URL: https://github.com/apache/cassandra/pull/1910
Add --resolve-ip option on 'nodetool gossipinfo' ([CASSANDRA-17934](https://issues.apache.org/jira/browse/CASSANDRA-17934))
### Command help update
```
maxim@309034f0356a:~/cassandra-dev$ ./bin/nodetool help gossipinfo
NAME
nodetool gossipinfo - Shows the gossip information for the cluster
SYNOPSIS
nodetool [(-h <host> | --host <host>)] [(-p <port> | --port <port>)]
... removed irrelevant lines ....
[(-u <username> | --username <username>)] gossipinfo
[(-r | --resolve-ip)]
OPTIONS
-h <host>, --host <host>
Node hostname or ip address
... removed irrelevant lines ...
-r, --resolve-ip
Show node domain names instead of IPs
-u <username>, --username <username>
Remote jmx agent username
```
### Behavior without new option
```
maxim@309034f0356a:~/cassandra-dev$ ./bin/nodetool gossipinfo
localhost/127.0.0.1
generation:1665667078
... removed irrelevant gossip info ...
maxim@309034f0356a:~/cassandra-dev$ ./bin/nodetool -pp gossipinfo
localhost/127.0.0.1:7000
generation:1665667078
... removed irrelevant gossip info ...
```
### Behavior with the new option
```
maxim@309034f0356a:~/cassandra-dev$ ./bin/nodetool gossipinfo --resolve-ip
localhost
generation:1665667078
... removed irrelevant gossip info ...
maxim@309034f0356a:~/cassandra-dev$ ./bin/nodetool -pp gossipinfo --resolve-ip
localhost:7000
generation:1665667078
... removed irrelevant gossip info ...
```
### Assumptions taken during development
**(1)** Class `org.apache.cassandra.gms.FailureDetectorMBean`.
The method `getAllEndpointStates` was marked as `@Deprecated`.
I've preserved this for the new method `getAllEndpointStatesWithResolveIp` as it does the same logic with addition of resolve ip.
**(2)** Class `org.apache.cassandra.locator.InetAddressAndPort`.
The method `public static String toString(InetAddress address, int port)` is commented specifically to return the following template:
`hostname / IP:port` (port is optional).
This is by design - to preserve `java.net.InetAddress`'s method `toString` template.
I've opted out to keep this and not remove the `hostname` part to keep the `java.net.InetAddress`'s convention.
patch by [Maxim Chanturiay](https://www.linkedin.com/in/maxim-ch); reviewed by <Reviewers> for CASSANDRA-17934
```
The [Cassandra Jira](https://issues.apache.org/jira/projects/CASSANDRA/issues/)
--
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
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #1910: Resolve IP option for Nodetool gossiping command
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #1910:
URL: https://github.com/apache/cassandra/pull/1910#discussion_r995460609
##########
src/java/org/apache/cassandra/tools/nodetool/GossipInfo.java:
##########
@@ -18,16 +18,20 @@
package org.apache.cassandra.tools.nodetool;
import io.airlift.airline.Command;
+import io.airlift.airline.Option;
import org.apache.cassandra.tools.NodeProbe;
import org.apache.cassandra.tools.NodeTool.NodeToolCmd;
@Command(name = "gossipinfo", description = "Shows the gossip information for the cluster")
public class GossipInfo extends NodeToolCmd
{
+ @Option(title = "resolve_ip", name = {"-r", "--resolve-ip"}, description = "Show node domain names instead of IPs")
Review Comment:
should not this be `Show node's domain names insteao of IPs` ? You can reformulate to "Show domain names of a node instead of its IPs".
Does a node have multiple IP? If it can have just one, plural in "IPs" should be changed to singular.
--
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
[GitHub] [cassandra] maxim-chn commented on a diff in pull request #1910: Resolve IP option for Nodetool gossiping command
Posted by GitBox <gi...@apache.org>.
maxim-chn commented on code in PR #1910:
URL: https://github.com/apache/cassandra/pull/1910#discussion_r995982850
##########
src/java/org/apache/cassandra/tools/NodeProbe.java:
##########
@@ -1426,7 +1426,19 @@ public Map<String, String> getSimpleStatesWithPort()
public String getGossipInfo(boolean withPort)
{
- return withPort ? fdProxy.getAllEndpointStatesWithPort() : fdProxy.getAllEndpointStates();
+ return getGossipInfo(withPort, false);
+ }
+
+ public String getGossipInfo(boolean withPort, boolean resolveIp)
+ {
+ if (resolveIp)
Review Comment:
@smiklosovic , thanks for pointing that out.
I'll keep it in mind for future commits.
The change was applied at [aa202f1](https://github.com/apache/cassandra/pull/1910/commits/aa202f1a971909f6d0682899d395ba9e7c325ccf).
I've seen the unit test `GossipInfoTest.java` (which invokes the changed method) pass.
--
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
[GitHub] [cassandra] smiklosovic closed pull request #1910: CASSANDRA-17934 4.1 Resolve IP option for Nodetool gossiping command
Posted by GitBox <gi...@apache.org>.
smiklosovic closed pull request #1910: CASSANDRA-17934 4.1 Resolve IP option for Nodetool gossiping command
URL: https://github.com/apache/cassandra/pull/1910
--
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
[GitHub] [cassandra] maxim-chn commented on a diff in pull request #1910: Resolve IP option for Nodetool gossiping command
Posted by GitBox <gi...@apache.org>.
maxim-chn commented on code in PR #1910:
URL: https://github.com/apache/cassandra/pull/1910#discussion_r995981599
##########
src/java/org/apache/cassandra/locator/InetAddressAndPort.java:
##########
@@ -122,6 +122,12 @@ public String getHostAddress(boolean withPort)
return hostAddress(this, withPort);
}
+ public String getHostName(boolean withPort)
+ {
+ StringBuilder sb = new StringBuilder(getHostName());
Review Comment:
@smiklosovic,
I've committed the new return statement at [adb93aa](https://github.com/apache/cassandra/pull/1910/commits/adb93aaa5722e6bd12b0b5e6eaaa864d511049c9) and seen that `InetAddressAndPortTest.java` unit test passed.
--
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
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #1910: Resolve IP option for Nodetool gossiping command
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #1910:
URL: https://github.com/apache/cassandra/pull/1910#discussion_r998509343
##########
src/java/org/apache/cassandra/gms/FailureDetectorMBean.java:
##########
@@ -32,7 +32,9 @@
public double getPhiConvictThreshold();
@Deprecated public String getAllEndpointStates();
+ @Deprecated public String getAllEndpointStatesWithResolveIp();
Review Comment:
This smells. I do not know what is that method good for when we deprecated it on commit. Why do we introduce something we deprecate? Maybe I am missing something?
--
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
[GitHub] [cassandra] maxim-chn commented on a diff in pull request #1910: Resolve IP option for Nodetool gossiping command
Posted by GitBox <gi...@apache.org>.
maxim-chn commented on code in PR #1910:
URL: https://github.com/apache/cassandra/pull/1910#discussion_r1000411728
##########
src/java/org/apache/cassandra/gms/FailureDetectorMBean.java:
##########
@@ -32,7 +32,9 @@
public double getPhiConvictThreshold();
@Deprecated public String getAllEndpointStates();
+ @Deprecated public String getAllEndpointStatesWithResolveIp();
Review Comment:
@smiklosovic , I completely agree.
I have opted to use `@Deprecated` because the method `getAllEndpointStates` (which represents the same logic except for resolving ip) was marked as `@Deprecated`.
I couldn't understand myself why the method `getAllEndpointStates()` was marked as `@Deprecated` in the first place. The method is in use in fact and I have not found substitute logic for it in the code.
The [original commit](https://github.com/apache/cassandra/commit/59b5b6bef0fa76bf5740b688fcd4d9cf525760d0#diff-7957ef3fe425d75793e597cc24c9f1429c6da5aac9dd3fa6169ff832fec81814) by Ariel Weisberg however, introduced the method `@Deprecated getAllEndpointStates()` from start. It was never not deprecated.
I have followed related ticket [CASSANDRA-7544](https://issues.apache.org/jira/browse/CASSANDRA-7544) to find a clue for `@Deprecated` - but there was none.
For what it's worth, I suggest to remove `@Deprecated` for both of these methods.
--
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
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #1910: Resolve IP option for Nodetool gossiping command
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #1910:
URL: https://github.com/apache/cassandra/pull/1910#discussion_r995451648
##########
src/java/org/apache/cassandra/gms/FailureDetector.java:
##########
@@ -107,20 +107,35 @@ private static long getInitialValue()
public String getAllEndpointStates()
{
- return getAllEndpointStates(false);
+ return getAllEndpointStates(false, false);
+ }
+
+ public String getAllEndpointStatesWithResolveIp()
Review Comment:
should not it be "...WithResolvedIp()" ? Notice "Resolved" instead of "Resolve". This change should be applicable everywhere in this patch.
--
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
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #1910: Resolve IP option for Nodetool gossiping command
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #1910:
URL: https://github.com/apache/cassandra/pull/1910#discussion_r995454220
##########
src/java/org/apache/cassandra/locator/InetAddressAndPort.java:
##########
@@ -122,6 +122,12 @@ public String getHostAddress(boolean withPort)
return hostAddress(this, withPort);
}
+ public String getHostName(boolean withPort)
+ {
+ StringBuilder sb = new StringBuilder(getHostName());
Review Comment:
This can be just simplified to this:
`return withPort ? String.format("%s:%s", getHostname(), getPort()) : getHostname();`
I do not think we need `StringBuilder` for this.
--
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
[GitHub] [cassandra] maxim-chn commented on a diff in pull request #1910: Resolve IP option for Nodetool gossiping command
Posted by GitBox <gi...@apache.org>.
maxim-chn commented on code in PR #1910:
URL: https://github.com/apache/cassandra/pull/1910#discussion_r995993791
##########
src/java/org/apache/cassandra/tools/nodetool/GossipInfo.java:
##########
@@ -18,16 +18,20 @@
package org.apache.cassandra.tools.nodetool;
import io.airlift.airline.Command;
+import io.airlift.airline.Option;
import org.apache.cassandra.tools.NodeProbe;
import org.apache.cassandra.tools.NodeTool.NodeToolCmd;
@Command(name = "gossipinfo", description = "Shows the gossip information for the cluster")
public class GossipInfo extends NodeToolCmd
{
+ @Option(title = "resolve_ip", name = {"-r", "--resolve-ip"}, description = "Show node domain names instead of IPs")
Review Comment:
@smiklosovic , I've basically copy-pasted the existing phrasing for `--resolve-ip` options at:
- https://cassandra.apache.org/doc/4.1/cassandra/tools/nodetool/status.html
- https://cassandra.apache.org/doc/4.1/cassandra/tools/nodetool/ring.html
I think the intention was that commands will output data about multiple nodes in production.
And IPs for these nodes will be resolved to their relevant domain names.
Should I still commit a singular phrasing? If yes, should I re-phrase along the way the `status` and `ring` commands to align all three of them to the same `--resolve-ip` option description?
--
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
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #1910: Resolve IP option for Nodetool gossiping command
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #1910:
URL: https://github.com/apache/cassandra/pull/1910#discussion_r995456431
##########
src/java/org/apache/cassandra/tools/NodeProbe.java:
##########
@@ -1426,7 +1426,19 @@ public Map<String, String> getSimpleStatesWithPort()
public String getGossipInfo(boolean withPort)
{
- return withPort ? fdProxy.getAllEndpointStatesWithPort() : fdProxy.getAllEndpointStates();
+ return getGossipInfo(withPort, false);
+ }
+
+ public String getGossipInfo(boolean withPort, boolean resolveIp)
+ {
+ if (resolveIp)
Review Comment:
if `if / else` is so simple, we tend to leave parenthesis completely, do this:
````
if (resolveIp)
return ...
else
return ...
````
--
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
[GitHub] [cassandra] maxim-chn commented on a diff in pull request #1910: Resolve IP option for Nodetool gossiping command
Posted by GitBox <gi...@apache.org>.
maxim-chn commented on code in PR #1910:
URL: https://github.com/apache/cassandra/pull/1910#discussion_r995996320
##########
src/java/org/apache/cassandra/gms/FailureDetector.java:
##########
@@ -107,20 +107,35 @@ private static long getInitialValue()
public String getAllEndpointStates()
{
- return getAllEndpointStates(false);
+ return getAllEndpointStates(false, false);
+ }
+
+ public String getAllEndpointStatesWithResolveIp()
Review Comment:
@smiklosovic ,
Thanks for the quick review!
It's my first open source merge request, I was really nervous if my implementation was even in the right direction 😅
By the way, I am not resolving any thread by myself.
Please tell me if it's ok to just leave a comment (and commit) and wait for a reviewer to resolve?
Or should I resolve a thread to which I have attached the commit with the fix?
--
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
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #1910: Resolve IP option for Nodetool gossiping command
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #1910:
URL: https://github.com/apache/cassandra/pull/1910#discussion_r995451648
##########
src/java/org/apache/cassandra/gms/FailureDetector.java:
##########
@@ -107,20 +107,35 @@ private static long getInitialValue()
public String getAllEndpointStates()
{
- return getAllEndpointStates(false);
+ return getAllEndpointStates(false, false);
+ }
+
+ public String getAllEndpointStatesWithResolveIp()
Review Comment:
should not it be "...WithResolvedIp()" ? Notice "Resolved" instead of "Resolve". This change should be applicable everywhere in this patch.
EDIT: you know what ... just leave it as it is.
--
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