This thread is clearly mostly not about the feature branch question, but sure...let's summarize the case for picocli (rather than airline-2, jcommander, and commons-cli) in a fresh, specifically titled thread. Then we can continue any library-agnostic discussion here if necessary.
On Mon, Jul 8, 2024 at 3:40 PM Brad <bscho...@gmail.com> wrote: > Many of the utilities in the tools directory (BulkLoader, SSTableExpoert, > etc) already use apache.commons.cli. > > On Mon, Jul 8, 2024 at 4:28 PM Dinesh Joshi <djo...@apache.org> wrote: > >> I agree about picking libraries on their merit but a major factor for any >> open source project should consider today is the possibility of >> unfavorable/hostile licensing changes. >> >> On Mon, Jul 8, 2024 at 1:15 PM Jon Haddad <j...@jonhaddad.com> wrote: >> >>> Without getting into the pros and cons of both libraries, I have to >>> point out there's something unsettling about making decisions about >>> libraries we used based on arbitrary rules an employer has put into place >>> on its employees. The project isn't governed by Apple, it's governed by >>> individual contributors to open source. >>> >>> We need to pick libraries based on their merits. Apple's draconian >>> rules should not prevent us from using the best option available. >>> >>> Jon >>> >>> >>> On Mon, Jul 8, 2024 at 12:57 PM Dinesh Joshi <djo...@apache.org> wrote: >>> >>>> I agree, having a DISCUSS thread with a specific subject line is less >>>> likely to be overlooked. >>>> >>>> One thing I'd like to note here is PicoCLI and Airline 2 are >>>> independent projects that are ALv2 licensed. A subset of the Cassandra >>>> contributors may have difficulty contributing to such projects due to >>>> preexisting policies that their employers may have in place. >>>> >>>> I am concerned about hostile licensing changes in the future which will >>>> necessitate another migration for us. That said, is there a specific reason >>>> to not consider Apache Commons CLI[1]? >>>> >>>> Dinesh >>>> >>>> [1] https://commons.apache.org/proper/commons-cli/ >>>> >>>> On Mon, Jul 8, 2024 at 10:22 AM David Capwell <dcapw...@apple.com> >>>> wrote: >>>> >>>>> I don't think that a separate thread would add extra visibility >>>>> >>>>> >>>>> Disagree. This thread is about adding a feature branch, so many could >>>>> ignore if they don’t care. The fact you are switching the library (and >>>>> which one) is something we have to hunt for. By having a new DISCUSS >>>>> thread it makes it clear which library you wish to add, and people can >>>>> sign >>>>> off if they care or not. >>>>> >>>>> I wouldn’t create this thread until you settle on which one you wish >>>>> to move forward with. >>>>> >>>>> Is adding the PicoCLI library as a project dependency getting any >>>>> objections >>>>> from the Community? >>>>> >>>>> >>>>> Thats the point of the new DISCUSS thread. By being very clear you >>>>> wish to add PicoCLI people can either validate we are allowed to, or raise >>>>> any objections. I have not really seen any pushback so far outside of 1 >>>>> case that wasn’t legally allowed to be used. >>>>> >>>>> Take a look at previous threads about adding different libraries. >>>>> >>>>> On Jul 8, 2024, at 7:58 AM, Caleb Rackliffe <calebrackli...@gmail.com> >>>>> wrote: >>>>> >>>>> +1 on picocli >>>>> >>>>> RE the feature branch, I would just maintain the feature branch in >>>>> your own fork to break out whatever "reviewable units" of code you want. >>>>> When all the incremental review is done (I have no problem going back and >>>>> forth), squash everything together, do whatever additional testing you >>>>> need, and commit. >>>>> >>>>> On Fri, Jul 5, 2024 at 10:40 AM Maxim Muzafarov <mmu...@apache.org> >>>>> wrote: >>>>> >>>>>> > Once you are happy with your chosen library, we need a DISCUSS >>>>>> thread to add this new library (current protocol). >>>>>> >>>>>> Thanks, David. This is a good point, do we need a separate DISCUSS >>>>>> thread or can we just use this one? I'm in favour of keeping the >>>>>> discussion in one place, especially when topics are closely related. I >>>>>> don't think that a separate thread would add extra visibility, but if >>>>>> that is the way the community has adopted - no problem at all, I'll >>>>>> repost. >>>>>> >>>>>> >>>>>> The reasons for replacing the Airlift/Airline [1] with the PicoCli [2] >>>>>> are as follows (in order of priority): >>>>>> >>>>>> 1. The library is under the Apache-2.0 License >>>>>> https://github.com/remkop/picocli?tab=Apache-2.0-1-ov-file#readme >>>>>> >>>>>> 2. The project is active and well-maintained (last release on 8 May >>>>>> 2024) >>>>>> https://github.com/remkop/picocli/releases >>>>>> >>>>>> 3. The library has ZERO dependencies, in some of the cases a single >>>>>> file can just be dropped into the sources (it's even pointed out in >>>>>> the documentation) >>>>>> https://picocli.info/#_add_as_source >>>>>> >>>>>> 4. Compared to the Airlift library, the PicoCLI uses the same markup >>>>>> design concepts, so we don't have to rewrite our command or make >>>>>> complex changes, which in turn minimizes the migration. >>>>>> >>>>>> >>>>>> Is adding the PicoCLI library as a project dependency getting any >>>>>> objections from the Community? Please, share your thoughts. >>>>>> >>>>>> There are a few other alternatives (commons-cli, airline2, jcommander) >>>>>> but they are not as well known and/or not as elegantly suited to our >>>>>> needs based on what we have now. >>>>>> >>>>>> >>>>>> [1] https://github.com/airlift/airline >>>>>> [2] https://github.com/remkop/picocli >>>>>> >>>>>> >>>>>> On Wed, 3 Jul 2024 at 22:27, David Capwell <dcapw...@apple.com> >>>>>> wrote: >>>>>> > >>>>>> > I don't personally think there is a strong need for a feature >>>>>> branch. If it makes it easy for you, please go ahead with a feature >>>>>> branch. >>>>>> > >>>>>> > >>>>>> > Agree, I don’t see the reason for a feature branch… feature branch >>>>>> just means the branch lives in apache domain rather than your own fork. >>>>>> You won’t be able to merge until you are done and you will need to keep >>>>>> rebasing over and over again. Even if multiple people are working on this >>>>>> you can work in your fork just fine (assuming you grant permissions). >>>>>> > >>>>>> > Another issue is that feature branches require the same level of >>>>>> commit process as every other main branch, where as personal branches >>>>>> don’t. This actually will slow you down as each commit now must be a >>>>>> JIRA, >>>>>> you go through review of each, must show a success CI, etc. >>>>>> > >>>>>> > Now, if you wish to split this into multiple steps that is fine, >>>>>> but the list of places is basically node tool (kinda has to go in at >>>>>> once) >>>>>> and small CLIs. If you wish to migrate the small ones in isolation >>>>>> first, >>>>>> I am cool with that merging to w/e branch the logic is targeting, but you >>>>>> won’t be able to break up node tool without breaking everything… but if >>>>>> you >>>>>> did this in your own fork then no one cares. >>>>>> > >>>>>> > I won’t block a feature branch, but just don’t see a clear “why” >>>>>> and only see cons. >>>>>> > >>>>>> > We are changing the command markup library, so there are two extra >>>>>> > things to be checked: >>>>>> > - We parse CLI arguments in the same way (as the parser is different >>>>>> > in a new library); >>>>>> > - The command help output is the same so that the user won't see >>>>>> any difference; >>>>>> > >>>>>> > >>>>>> > Personally I would POC a limited node tool change with JVM dtest as >>>>>> we require passing the output to the test (the prototypes you listed >>>>>> doesn’t include JVM Dtest integration). If one library makes this more >>>>>> annoying, then do we care about fancy new features we don’t use when it >>>>>> makes the features we do use harder? If you start with the smaller tools >>>>>> first then spend a ton of time migrating node tool then find JVM dtest is >>>>>> broken, then you will spend so much more time fixing this, I would >>>>>> strongly >>>>>> recommend doing some throw away POC to make sure w/e way you go won’t >>>>>> break >>>>>> JVM Dtest’s node tool support. >>>>>> > >>>>>> > Once you are fine with your selected library, we will need a >>>>>> DISCUSS thread to add that new library (current protocol). This mostly >>>>>> just makes the pick more visible, and normally we only check simple >>>>>> things >>>>>> like “are we legally allowed to use” and “is this project dead?”. >>>>>> > >>>>>> > >>>>>> > On Jul 3, 2024, at 6:06 AM, Maxim Muzafarov <mmu...@apache.org> >>>>>> wrote: >>>>>> > >>>>>> > Thank you all for your comments, >>>>>> > >>>>>> > I want to stress, that these changes won't affect the input/output >>>>>> > formatting of commands, ensuring everything is the same. >>>>>> > >>>>>> > We are changing the command markup library, so there are two extra >>>>>> > things to be checked: >>>>>> > - We parse CLI arguments in the same way (as the parser is different >>>>>> > in a new library); >>>>>> > - The command help output is the same so that the user won't see >>>>>> any difference; >>>>>> > >>>>>> > Additional tests cover both cases. >>>>>> > >>>>>> > On Mon, 1 Jul 2024 at 20:08, Dinesh Joshi <djo...@apache.org> >>>>>> wrote: >>>>>> > >>>>>> > >>>>>> > I don't personally think there is a strong need for a feature >>>>>> branch. If it makes it easy for you, please go ahead with a feature >>>>>> branch. >>>>>> > >>>>>> > One thing I had raised in the past was the desire to have a flag >>>>>> that would generate machine readable output for nodetool commands. If >>>>>> this >>>>>> can be done with a minor incremental effort, it would definitely reduce >>>>>> the >>>>>> burden on operators / integrations that rely on the nodetool output. As I >>>>>> have earlier indicated in the past, relying on human readable output for >>>>>> CLI tools like nodetool is fragile and providing a JSON output as an >>>>>> alternative is a great first step in eliminating that dependency. I'm >>>>>> just >>>>>> curious about the level of effort. If it is too much or too invasive, we >>>>>> can consider producing JSON output for inclusion in the next major >>>>>> release. >>>>>> > >>>>>> > On Fri, Jun 28, 2024 at 6:47 AM Maxim Muzafarov <mmu...@apache.org> >>>>>> wrote: >>>>>> > >>>>>> > >>>>>> > Hello everyone, >>>>>> > >>>>>> > >>>>>> > The nodetool relies on the airlift/airline library to mark up the >>>>>> CLI >>>>>> > commands used to manage Cassandra, which are part of our public API. >>>>>> > This library is no longer maintained, so we need to update it >>>>>> anyway, >>>>>> > and the good news is that we already have several good alternatives: >>>>>> > airline-2 [3] or picocli [2]. >>>>>> > >>>>>> > In this message, I'm mainly talking about CASSANDRA-17445 [4], which >>>>>> > refers to the problem and is a prerequisite for a larger CEP-38 CQL >>>>>> > Management API [5]. It doesn't make sense to use annotations from >>>>>> the >>>>>> > deprecated library to build a new API, so this is another reason to >>>>>> > update the library as soon as possible and do some inherently small >>>>>> > code refactoring required for the CEP-38. >>>>>> > >>>>>> > In addition to being widely used and well supported, the Picocli >>>>>> > library offers the following advantages for us: >>>>>> > - We can detach the jmx-specific parameters from the commands so >>>>>> that >>>>>> > they can be reused in other APIs (e.g. without host, port) while >>>>>> > remaining backwards compatible; >>>>>> > - We can set up nodetool's autocompletion after the migration with >>>>>> > minimal effort; >>>>>> > - There is a good Picocli ecosystem of tools that we can use to >>>>>> > simplify our codebase, e.g. generate man pages tool to make our CLIs >>>>>> > more Unix friendly [7]; >>>>>> > >>>>>> > >>>>>> > = Prototype = >>>>>> > >>>>>> > I have a working prototype [8] that shows what the result will look >>>>>> > like. The prototype includes: >>>>>> > - Tests between the execution of commands via the nodetool and >>>>>> nodtoolv2; >>>>>> > - 5 out of 164 nodetool commands have been moved so far, to show the >>>>>> > refactoring we need to do to the command's body; >>>>>> > - The command help output under for the nodetoolv2 is the same as it >>>>>> > is currently for the nodetool and this is the default, however a >>>>>> > "cassandra.cli.picocli.layout" is added to switch to the Picocli >>>>>> > defaults; >>>>>> > - You can also see that the colour scheme is applied by the Picocli >>>>>> > out of the box, and this is how it looks [9]; >>>>>> > - The nodetoolv2 is called first when the shell is triggered, and if >>>>>> > the nodetoolv2 doesn't contain the command it needs yet, it falls >>>>>> back >>>>>> > to the nodetool and the old argument parser; >>>>>> > >>>>>> > >>>>>> > Since the number of commands is quite large (164), I'd like to >>>>>> create >>>>>> > a feature branch and move all the commands one at a time, while >>>>>> > keeping the output backwards by applying additional tests at the >>>>>> same >>>>>> > time and checking that the CI is always green. I think the "feature >>>>>> > branch" approach will be less stressful for us since it focuses on >>>>>> > requiring a review of only tedious changes to the feature branch, >>>>>> > rather than reviewing the 15k line patch. >>>>>> > >>>>>> > >>>>>> > Anyway, I am open to any suggestions and advice based on your >>>>>> > experience and best practices for this case. Looking forward to your >>>>>> > thoughts and suggestions. >>>>>> > >>>>>> > >>>>>> > >>>>>> > [1] https://github.com/airlift/airline >>>>>> > [2] https://picocli.info/ >>>>>> > [3] https://github.com/rvesse/airline >>>>>> > [4] https://issues.apache.org/jira/browse/CASSANDRA-17445 >>>>>> > [5] >>>>>> https://cwiki.apache.org/confluence/display/CASSANDRA/CEP-38%3A+CQL+Management+API >>>>>> > [6] >>>>>> https://github.com/apache/cassandra/pull/2497/files#diff-acdd5f29d28df5c02f4bfc933528f084508b4923112e312e68a4aff7df973bce >>>>>> > [7] https://picocli.info/man/gen-manpage.html >>>>>> > [8] https://github.com/apache/cassandra/pull/2497/files >>>>>> > [9] >>>>>> https://github.com/apache/cassandra/assets/3415046/57b14ae0-ff59-43d2-b542-10d3218ae075 >>>>>> > >>>>>> > >>>>>> >>>>> >>>>>
