Hi Maxim, I took yet another look after my initial review some time ago and I still do not see any issues with it.
I appreciate that by default it behaves exactly the same way as before and one has to just flip a switch (env property / system property) to start to use another layout. Arguments / parameters are behaving the same way as well, no matter if picocli or legacy airline is used which is just great as the end user does not notice anything. I would just stay on legacy layout for a while and if no other issues are found we might contemplate about making the switch but I think we should just go over some phase of having legacy as default and people might opt-in into new stuff (that's what I will be doing at least). Not sure what needs to happen in order to merge it, I think one more committer taking a look would be great. Regards On Fri, May 9, 2025 at 9:45 AM Maxim Muzafarov <mmu...@apache.org> wrote: > Hello everyone, > > The commands have been migrated to picocli and are ready for review, > and we need a second committer to review them. > Would anyone be able to help? > > Key points: > - All the commands are backwards-compatible with everything we have in > the trunk now (including the accord commands); > - The commands help output also match the trunk (no difference from > the UX point of view); > - Test coverage has also been significantly increased (most of the > changes are new tests); > > https://github.com/apache/cassandra/pull/2497/files > https://issues.apache.org/jira/browse/CASSANDRA-17445 > > On Mon, 15 Jul 2024 at 20:53, Maxim Muzafarov <mmu...@apache.org> wrote: > > > > Hello everyone, > > > > > > I want to continue the discussion that was originally started here > > [2], however, it's better to move it to a new thread with an > > appropriate title, so that everyone is aware of the replacement > > library we're trying to agree on. > > > > The question is: > > Does everyone agree with using Picocli as an airlift/airline > > replacement for our cli tools? > > The prototype to look at is here [1]. > > > > > > The reasons are as follows: > > > > Why to replace? > > > > There are several cli tools that rely on the airlift/airline library > > to mark up the commands: NodeTool, JMXTool, FullQueryLogTool, > > CompactionStress (with the size of the NodeTool dominating the rest of > > the tools). The airline is no longer maintained, so we will have to > > update it sooner or later anyway. > > > > > > What criteria? > > > > Before we dive into the pros and cons of each candidate, I think we > > have to formulate criteria for the libraries we are considering, based > > on what we already have in the source code (from Cassandra's > > perspective). This in itself limits the libraries we can consider. > > > > Criteria can be as follows: > > - Library licensing, including risks that it may change in the future > > (the asf libs are the safest for us from this perspective); > > - Similarity of library design (to the airline). This means that the > > closer the libraries are, the easier it is to migrate to them, and the > > easier it is to get guarantees that we haven't broken anything. The > > further away the libraries are, the more extra code and testing we > > need; > > - Backward compatibility. The ideal case is where the user doesn't > > even notice that a different library is being used under the hood. > > This includes both the help output and command output. > > > > Of course, all libraries need to be known and well-maintained. > > > > What candidates? > > > > > > Picocli > > https://picocli.info/ > > > > This is the well-known cli library under the Apache 2.0 license, which > > is similar to what we have in source code right now. This also means > > that the amount of changes (despite the number of the commands) > > required to migrate what we have is quite small. > > In particular, I would like to point out that: > > - It allows us to unbind the jmx-specific command options from the > > commands themselves, so that they can be reused in other APIs (my > > goal); > > - We can customize the help output so that the user doesn't notice > > anything while using of the nodetool; > > - The cli parser is the same as what we now do with cli arguments. > > > > This makes the library a good candidate, but leaves open the question > > of changing the license of the lib in the future. However, these risks > > are relatively small because the CLI library is not a monetizable > > thing, as I believe. We can also mitigate the risks copying the lib to > > sources, as it mentioned here: > > https://picocli.info/#_getting_started > > > > > > commons-cli > > https://commons.apache.org/proper/commons-cli/ > > > > In terms of licenses, it is the easiest candidate for us to use as > > it's under the asf, and in fact the library is already used in e.g. > > BulkLoader, SSTableExpoert. > > However, I'd like to point out the following disadvantages the library > > has for our case: > > - This is not a drop-in replacement for the airline commands, as the > > lib does not have annotation for markup commands. We have to flesh out > > all the options we have as java classes, or create our owns; > > - Subcommands have to be supported manually, which requires extra > > effort to adopt the cli parser (correct me if I'm wrong here). We have > > at least several subcommands in the NodeTool e.g. cms describe, cms > > snapshot; > > - Apart from parsing the cli arguments, we need to manually initialize > > the command class and set the input arguments we have. > > > > > > JComannder > > https://jcommander.org/ > > > > The library is licensed under the Apache 2.0 license, so the situation > > is the same as for Picocli. Here I'd like to point out a few things I > > encountered while prototyping: > > - Unbinding the jmx-specific options from commands is quite tricky and > > requires touching an internal API (which I won't do). Option > > inheritance is not the way to go if we want to have a clear command > > hierarchy regardless of the API used. > > - We won't be able to inject a Logger (the Output class in terms of > > NodeTool) or other abstractions (e.g. MBeans) directly into the > > commands, because it doesn't support dependency injection. This is > > also part of the activity of reusing the commands in other APIs, for > > instance to execute them via CQL; > > > > More basic in comparison to the Picocli, focusing primarily on simple > > annotation-based parsing and subcommands, and won't allow us to reuse > > the commands outside of the cli. > > > > > > airline2 > > https://github.com/rvesse/airline > > > > The library is licensed under the Apache 2.0 license, and this is an > > attempt to rebuild the original airline library. Currently, this is > > not a drop-in replacement, as it has some minor API differences from > > the original library. It is also not a better choice for us, as it has > > the same drawbacks I mentioned for the previous alternatives, e.g. not > > possible to unbind the specific options from the command and use them > > only when commands are called from the cli. > > > > > > > > > > [1] https://github.com/apache/cassandra/pull/2497/files > > [2] https://lists.apache.org/thread/m9wfb3gdo9s210824c9rm2ojc9qv9412 >
