You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "smiklosovic (via GitHub)" <gi...@apache.org> on 2024/12/15 20:38:56 UTC
[PR] CASSANDRA-20104 sorting of nodetool status output - further refactorings and improvements [cassandra]
smiklosovic opened a new pull request, #3744:
URL: https://github.com/apache/cassandra/pull/3744
Thanks for sending a pull request! Here are some tips if you're new here:
* Ensure you have added or run the [appropriate tests](https://cassandra.apache.org/_/development/testing.html) for your PR.
* Be sure to keep the PR description updated to reflect all changes.
* Write your PR title to summarize what this PR proposes.
* If possible, provide a concise example to reproduce the issue for a faster review.
* Read our [contributor guidelines](https://cassandra.apache.org/_/development/index.html)
* If you're making a documentation change, see our [guide to documentation contribution](https://cassandra.apache.org/_/development/documentation.html)
Commit messages should follow the following format:
```
<One sentence description, usually Jira title or CHANGES.txt summary>
<Optional lengthier description (context on patch)>
patch by <Authors>; reviewed by <Reviewers> for CASSANDRA-#####
Co-authored-by: Name1 <email1>
Co-authored-by: Name2 <email2>
```
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
Re: [PR] CASSANDRA-20104 sorting of nodetool status output - further refactorings and improvements [cassandra]
Posted by "manish-m-pillai (via GitHub)" <gi...@apache.org>.
manish-m-pillai commented on code in PR #3744:
URL: https://github.com/apache/cassandra/pull/3744#discussion_r1886370702
##########
src/java/org/apache/cassandra/tools/nodetool/Status.java:
##########
@@ -48,6 +56,21 @@ public class Status extends NodeToolCmd
@Option(title = "resolve_ip", name = {"-r", "--resolve-ip"}, description = "Show node domain names instead of IPs")
private boolean resolveIp = false;
+ @Option(title = "sort",
+ name = {"-s", "--sort"},
+ description = "Sort by one of 'ip', 'host', 'load', 'owns', 'id', 'rack' or 'state'. " +
+ "Default ordering is ascending for 'ip', 'host', 'id', 'token', 'rack' and descending for 'load', 'owns', 'state'. " +
+ "Sorting by token is possible only when cluster does not use vnodes. When using vnodes, default " +
+ "sorting is by id otherwise by token.",
+ allowedValues = {"ip", "host", "load", "owns", "id", "rack", "state", "none"})
Review Comment:
If we are also including token do we not have to include token in the allowed values?
--
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
Re: [PR] CASSANDRA-20104 sorting of nodetool status output - further refactorings and improvements [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic closed pull request #3744: CASSANDRA-20104 sorting of nodetool status output - further refactorings and improvements
URL: https://github.com/apache/cassandra/pull/3744
--
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
Re: [PR] CASSANDRA-20104 sorting of nodetool status output - further refactorings and improvements [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #3744:
URL: https://github.com/apache/cassandra/pull/3744#discussion_r1885853611
##########
src/java/org/apache/cassandra/tools/nodetool/Status.java:
##########
@@ -98,25 +121,63 @@ public void execute(NodeProbe probe)
SortedMap<String, SetHostStatWithPort> dcs = NodeTool.getOwnershipByDcWithPort(probe, resolveIp, tokensToEndpoints, ownerships);
+ int nodesOfTokens = tokensToEndpoints.values().size();
+
// More tokens than nodes (aka vnodes)?
- if (dcs.size() < tokensToEndpoints.size())
+ if (hostIDMap.size() < nodesOfTokens)
Review Comment:
I strongly believe this is a bug. The code as it was does not make any sense.
--
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
Re: [PR] CASSANDRA-20104 sorting of nodetool status output - further refactorings and improvements [cassandra]
Posted by "jrwest (via GitHub)" <gi...@apache.org>.
jrwest commented on code in PR #3744:
URL: https://github.com/apache/cassandra/pull/3744#discussion_r1888879059
##########
src/java/org/apache/cassandra/tools/nodetool/Status.java:
##########
@@ -48,15 +60,71 @@ public class Status extends NodeToolCmd
@Option(title = "resolve_ip", name = {"-r", "--resolve-ip"}, description = "Show node domain names instead of IPs")
private boolean resolveIp = false;
+ @Option(title = "sort",
+ name = {"-s", "--sort"},
+ description = "Sort by one of 'ip', 'host', 'load', 'owns', 'id', 'rack' or 'state'. " +
+ "Default ordering is ascending for 'ip', 'host', 'id', 'token', 'rack' and descending for 'load', 'owns', 'state'. " +
+ "Sorting by token is possible only when cluster does not use vnodes. When using vnodes, default " +
+ "sorting is by id otherwise by token.",
+ allowedValues = {"ip", "host", "load", "owns", "id", "rack", "state"})
+ private String sortBy = null;
+
+ @Option(title = "sort_order",
+ name = {"-o", "--order"},
+ description = "Sorting order: 'asc' for ascending, 'desc' for descending.",
+ allowedValues = {"asc", "desc"})
+ private String sortOrder = null;
+
private boolean isTokenPerNode = true;
private Collection<String> joiningNodes, leavingNodes, movingNodes, liveNodes, unreachableNodes;
private Map<String, String> loadMap, hostIDMap;
private EndpointSnitchInfoMBean epSnitchInfo;
+ private SortBy parseSortBy(String setSortBy, PrintStream out)
Review Comment:
very minor nit: this function and `parseSortOrder` could be `static`
##########
src/java/org/apache/cassandra/tools/nodetool/Status.java:
##########
@@ -98,25 +121,63 @@ public void execute(NodeProbe probe)
SortedMap<String, SetHostStatWithPort> dcs = NodeTool.getOwnershipByDcWithPort(probe, resolveIp, tokensToEndpoints, ownerships);
+ int nodesOfTokens = tokensToEndpoints.values().size();
+
// More tokens than nodes (aka vnodes)?
- if (dcs.size() < tokensToEndpoints.size())
+ if (hostIDMap.size() < nodesOfTokens)
Review Comment:
I agree, especially given the comment above it. i think it was intended to check the size of the values which I see is what your change does 👍
##########
src/java/org/apache/cassandra/tools/nodetool/Status.java:
##########
@@ -48,15 +60,71 @@ public class Status extends NodeToolCmd
@Option(title = "resolve_ip", name = {"-r", "--resolve-ip"}, description = "Show node domain names instead of IPs")
private boolean resolveIp = false;
+ @Option(title = "sort",
+ name = {"-s", "--sort"},
+ description = "Sort by one of 'ip', 'host', 'load', 'owns', 'id', 'rack' or 'state'. " +
+ "Default ordering is ascending for 'ip', 'host', 'id', 'token', 'rack' and descending for 'load', 'owns', 'state'. " +
+ "Sorting by token is possible only when cluster does not use vnodes. When using vnodes, default " +
+ "sorting is by id otherwise by token.",
+ allowedValues = {"ip", "host", "load", "owns", "id", "rack", "state"})
+ private String sortBy = null;
+
+ @Option(title = "sort_order",
+ name = {"-o", "--order"},
+ description = "Sorting order: 'asc' for ascending, 'desc' for descending.",
+ allowedValues = {"asc", "desc"})
+ private String sortOrder = null;
+
private boolean isTokenPerNode = true;
private Collection<String> joiningNodes, leavingNodes, movingNodes, liveNodes, unreachableNodes;
private Map<String, String> loadMap, hostIDMap;
private EndpointSnitchInfoMBean epSnitchInfo;
+ private SortBy parseSortBy(String setSortBy, PrintStream out)
Review Comment:
also really minor nti: would personaly prefer to see these down by their respective `enum` and below execute for readability but again super nit.
##########
src/java/org/apache/cassandra/tools/nodetool/Status.java:
##########
@@ -86,37 +154,75 @@ public void execute(NodeProbe probe)
}
catch (Exception ex)
{
- out.printf("%nError: %s%n", ex.getMessage());
+ errOut.printf("%nError: %s%n", ex.getMessage());
System.exit(1);
}
}
catch (IllegalArgumentException ex)
{
- out.printf("%nError: %s%n", ex.getMessage());
+ errOut.printf("%nError: %s%n", ex.getMessage());
System.exit(1);
}
SortedMap<String, SetHostStatWithPort> dcs = NodeTool.getOwnershipByDcWithPort(probe, resolveIp, tokensToEndpoints, ownerships);
+ int nodesOfTokens = tokensToEndpoints.values().size();
+
// More tokens than nodes (aka vnodes)?
- if (dcs.size() < tokensToEndpoints.size())
+ if (hostIDMap.size() < nodesOfTokens)
isTokenPerNode = false;
+ if (sortBy == null)
+ {
+ if (isTokenPerNode)
+ sortBy = SortBy.token;
+ else
+ sortBy = SortBy.id;
+ }
+ else if (!isTokenPerNode && sortBy == SortBy.token)
+ {
+ errOut.printf("%nError: Can not sort by token when there is not token per node.%n");
+ System.exit(1);
+ }
+ else if (!resolveIp && sortBy == SortBy.host)
+ {
+ errOut.printf("%nError: Can not sort by host when there is not -r/--resolve-ip flag used.%n");
+ System.exit(1);
+ }
+
// Datacenters
for (Map.Entry<String, SetHostStatWithPort> dc : dcs.entrySet())
{
TableBuilder tableBuilder = sharedTable.next();
addNodesHeader(hasEffectiveOwns, tableBuilder);
- ArrayListMultimap<String, HostStatWithPort> hostToTokens = ArrayListMultimap.create();
+ ArrayListMultimap<InetAddressAndPort, HostStatWithPort> hostToTokens = ArrayListMultimap.create();
for (HostStatWithPort stat : dc.getValue())
- hostToTokens.put(stat.endpointWithPort.getHostAddressAndPort(), stat);
+ hostToTokens.put(stat.endpointWithPort, stat);
Review Comment:
More a note to myself here: this is safe because we get `equals/hashCode` via `InetAddressAndPort`s parent class `InetSocketAddress`
##########
src/java/org/apache/cassandra/tools/nodetool/Status.java:
##########
@@ -48,15 +60,71 @@ public class Status extends NodeToolCmd
@Option(title = "resolve_ip", name = {"-r", "--resolve-ip"}, description = "Show node domain names instead of IPs")
private boolean resolveIp = false;
+ @Option(title = "sort",
+ name = {"-s", "--sort"},
+ description = "Sort by one of 'ip', 'host', 'load', 'owns', 'id', 'rack' or 'state'. " +
+ "Default ordering is ascending for 'ip', 'host', 'id', 'token', 'rack' and descending for 'load', 'owns', 'state'. " +
+ "Sorting by token is possible only when cluster does not use vnodes. When using vnodes, default " +
+ "sorting is by id otherwise by token.",
+ allowedValues = {"ip", "host", "load", "owns", "id", "rack", "state"})
+ private String sortBy = null;
+
+ @Option(title = "sort_order",
+ name = {"-o", "--order"},
+ description = "Sorting order: 'asc' for ascending, 'desc' for descending.",
+ allowedValues = {"asc", "desc"})
+ private String sortOrder = null;
+
private boolean isTokenPerNode = true;
private Collection<String> joiningNodes, leavingNodes, movingNodes, liveNodes, unreachableNodes;
private Map<String, String> loadMap, hostIDMap;
private EndpointSnitchInfoMBean epSnitchInfo;
+ private SortBy parseSortBy(String setSortBy, PrintStream out)
+ {
+ if (setSortBy == null)
+ return null;
+
+ try
+ {
+ return SortBy.valueOf(LocalizeString.toLowerCaseLocalized(setSortBy));
+ }
+ catch (IllegalArgumentException ex)
+ {
+ String enabledValues = Arrays.stream(SortBy.values()).map(SortBy::name).collect(Collectors.joining(", "));
+ out.printf("%nError: Illegal value for -s/--sort used: '" + setSortBy + "'. Supported values are " + enabledValues + ".%n");
+ System.exit(1);
+ return null;
+ }
+ }
+
+ private SortOrder parseSortOrder(String setSortOrder, PrintStream out)
+ {
+ if (setSortOrder == null)
+ return null;
+
+ try
+ {
+ return SortOrder.valueOf(LocalizeString.toLowerCaseLocalized(setSortOrder));
+ }
+ catch (IllegalArgumentException ex)
+ {
+ String enabledValues = Arrays.stream(SortOrder.values()).map(SortOrder::name).collect(Collectors.joining(", "));
+ out.printf("%nError: Illegal value for -o/--order used: '" + setSortOrder + "'. Supported values are " + enabledValues + ".%n");
+ System.exit(1);
+ return null;
+ }
+ }
+
@Override
public void execute(NodeProbe probe)
{
PrintStream out = probe.output().out;
+ PrintStream errOut = probe.output().err;
Review Comment:
Good catch. It would be nice if Nodetool handled this better but thats a matter for another ticket.
--
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
Re: [PR] CASSANDRA-20104 sorting of nodetool status output - further refactorings and improvements [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #3744:
URL: https://github.com/apache/cassandra/pull/3744#discussion_r1886589779
##########
src/java/org/apache/cassandra/tools/nodetool/Status.java:
##########
@@ -48,15 +60,71 @@ public class Status extends NodeToolCmd
@Option(title = "resolve_ip", name = {"-r", "--resolve-ip"}, description = "Show node domain names instead of IPs")
private boolean resolveIp = false;
+ @Option(title = "sort",
+ name = {"-s", "--sort"},
+ description = "Sort by one of 'ip', 'host', 'load', 'owns', 'id', 'rack' or 'state'. " +
+ "Default ordering is ascending for 'ip', 'host', 'id', 'token', 'rack' and descending for 'load', 'owns', 'state'. " +
+ "Sorting by token is possible only when cluster does not use vnodes. When using vnodes, default " +
+ "sorting is by id otherwise by token.",
+ allowedValues = {"ip", "host", "load", "owns", "id", "rack", "state"})
+ private String sortBy = null;
+
+ @Option(title = "sort_order",
+ name = {"-o", "--order"},
+ description = "Sorting order: 'asc' for ascending, 'desc' for descending.",
+ allowedValues = {"asc", "desc"})
+ private String sortOrder = null;
+
private boolean isTokenPerNode = true;
private Collection<String> joiningNodes, leavingNodes, movingNodes, liveNodes, unreachableNodes;
private Map<String, String> loadMap, hostIDMap;
private EndpointSnitchInfoMBean epSnitchInfo;
+ private SortBy parseSortBy(String setSortBy, PrintStream out)
+ {
+ if (setSortBy == null)
+ return null;
+
+ try
+ {
+ return SortBy.valueOf(LocalizeString.toLowerCaseLocalized(setSortBy));
+ }
+ catch (IllegalArgumentException ex)
+ {
+ String enabledValues = Arrays.stream(SortBy.values()).map(SortBy::name).collect(Collectors.joining(", "));
+ out.printf("%nError: Illegal value for -s/--sort used: '" + setSortBy + "'. Supported values are " + enabledValues + ".%n");
+ System.exit(1);
+ return null;
+ }
+ }
+
+ private SortOrder parseSortOrder(String setSortOrder, PrintStream out)
+ {
+ if (setSortOrder == null)
+ return null;
+
+ try
+ {
+ return SortOrder.valueOf(LocalizeString.toLowerCaseLocalized(setSortOrder));
+ }
+ catch (IllegalArgumentException ex)
+ {
+ String enabledValues = Arrays.stream(SortOrder.values()).map(SortOrder::name).collect(Collectors.joining(", "));
+ out.printf("%nError: Illegal value for -o/--order used: '" + setSortOrder + "'. Supported values are " + enabledValues + ".%n");
+ System.exit(1);
+ return null;
+ }
+ }
+
@Override
public void execute(NodeProbe probe)
{
PrintStream out = probe.output().out;
+ PrintStream errOut = probe.output().err;
Review Comment:
We were wrongly sending errors to out instead of err output across this class. This is now fixed.
--
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