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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
