Hi Maxim, I will take a look too. Regards, Dmitry
On Tue, 13 May 2025 at 08:59, Štefan Miklošovič <smikloso...@apache.org> wrote: > 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 >> > -- Dmitry Konstantinov
