CASSANDRA-20104: Sort output in nodetool status

[manish-m-pillai]

CASSANDRA-20104: Sort output in nodetool status

Sort output in nodetool status for improved readability

This patch sorts the output of the `nodetool status` command to provide a more organized and readable display, making it easier for users to interpret node status information.

Use -s or --sort option with any of the following : [ip, load, owns, id, rack, state]
It also includes an -o or --order option to specifically sort it in ascending or descending order : [asc for ascending and desc or descending]

if order is not specified it takes the default order as : asc for  'ip', 'id', 'rack' and desc for 'load', 'owns', 'state'

https://issues.apache.org/jira/browse/CASSANDRA-20104

[bbotella]

Thanks a lot for the patch @manish-m-pillai! I left some comments. Let me know if you need any clarification!

[manish-m-pillai]

Hi @bbotella I have made changes in the code as suggested.

[manish-m-pillai]

Hi @smiklosovic I have updated the formatting and changes that you have suggested.

[smiklosovic]

@manish-m-pillai if you accept this change then we can start to build this patch in CI

instaclustr@a58c9a7

What I did was that SortBy enum takes default sorting direction and then it implements abstract method which is sorting.

Then you can just take sortBy and sort very comfortably by one line.

What I like the most is that we are logically coupling sorting logic to respective sorting enum. Currently we do have an enum, but methods are separated from that and it is more complex than it could be.

[manish-m-pillai]

I like this approach of using enum for sorting.

[manish-m-pillai]

@smiklosovic I have updated the code for the above approach

[smiklosovic]

@manish-m-pillai great, I will start CI and I will post the results in the ticket.

[bbotella]

@manish-m-pillai if you accept this change then we can start to build this patch in CI

instaclustr@a58c9a7

What I did was that SortBy enum takes default sorting direction and then it implements abstract method which is sorting.

Then you can just take sortBy and sort very comfortably by one line.

What I like the most is that we are logically coupling sorting logic to respective sorting enum. Currently we do have an enum, but methods are separated from that and it is more complex than it could be.

Nice!

[jrwest]

Some early comments. Still need to read the tests and double check some of the changes in execute. Will also take it for a spin on a 3 node cluster.

[jrwest]

Making these values enums makes the error messages when you pass an invalid value a bit opaque. On one hand, I would say its fine to leave it because the output tells you to check nodetool help which does tell you the valid values. OTOH, having it tell you the valid values in the error message would be a better user experience (and a removal of a rough edge nodetool often has). To fix this though I think we would need to take the arguments as strings and then parse them in the body of the command so we could catch the exception and throw our own error message:

$ nt status --sort blah
nodetool: sort: can not convert "blah" to a SortBy
See 'nodetool help' or 'nodetool help <command>'.

$ nt status --sort ip --order blah
nodetool: sort_order: can not convert "blah" to a SortOrder
See 'nodetool help' or 'nodetool help <command>'.

[smiklosovic]

fixed in https://github.com/apache/cassandra/pull/3744/files

[jrwest]

If I am reading these right, each implementation duplicates a lot of the same code in their sort implementations. The only difference is the comparator. We could instead define the comparator and have one sort implementation which would reduce a lot of duplication and LOC.

[jrwest]

I just noticed there was some early discussion on this as well. I definitely like how its now split up per sort column but don't think we need the duplication of e.g. data.entrySet().stream() or in the collect call at the end either.

[smiklosovic]

that is already covered in the other PR.