aurora-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Maxim Khutornenko <ma...@apache.org>
Subject Re: [PROPOSAL] Replace commons-args
Date Fri, 08 Jan 2016 18:25:05 GMT
"Can you turn that concern into a more firm stance?" - you mastered
the original library and I fully trust you with the new one :)

+1.

On Thu, Jan 7, 2016 at 6:58 PM, Bill Farner <wfarner@apache.org> wrote:
> Can you turn that concern into a more firm stance?
>
> FWIW if you look closely, there's really only about 100 lines of greenfield
> code in /r/42042 (CommandLineParameters.java, sure to grow of course).
> There's a lot of bulk due to splitting out parser classes; commons-args has
> the same.
>
> I should have been clear - i'm not married to this.  Mostly scratching an
> old itch, i won't be upset if it's more churn than it's worth at this time.
>  (I _am_ tired of the gradle and intellij issues though.)
>
> On Thu, Jan 7, 2016 at 6:31 PM, Maxim Khutornenko <maxim@apache.org> wrote:
>
>> I am a big +1 to the refactoring if only for testability reasons.
>>
>> My only concern is the amount of supporting code that your POC
>> resulted in https://reviews.apache.org/r/42042. It may feel like
>> trading code with Commons parser at the end, which is battle proven
>> and well tested. That said, arg parsing is generally a well scoped
>> problem, so it should be relatively easy to cover major testing
>> scenarios.
>>
>> On Thu, Jan 7, 2016 at 5:18 PM, Bill Farner <wfarner@apache.org> wrote:
>> > All,
>> >
>> > I propose that we replace our current command line args system.  For
>> those
>> > unaware, the args system [1] we currently use was forked out of Twitter
>> > Commons several months ago.  The goal when forking was to adapt/maintain
>> or
>> > eventually replace these libraries.  Given the code volume and complexity
>> > of commons-args, i propose we replace it and address some issues along
>> the
>> > way.
>> >
>> > I suggest several goals:
>> > a. sidestep current development hurdles we have encountered with the args
>> > system (intellij/gradle not working nicely with apt)
>> > b. encourage better testability of Module classes by always injecting all
>> > args
>> > c. leverage a well-maintained third-party argument parsing library
>> > d. stretch: enable user-friendly features like logical option groups for
>> > better help/usage output
>> > e. stretch: enable alternative configuration inputs like a configuration
>> > file or environment variables
>> >
>> > (b) is currently an issue because command line arguments are driven from
>> > pseudo-constants within the code, for example:
>> >
>> >     @NotNull
>> >     @CmdLine(name = "cluster_name", help = "Name to identify the cluster
>> > being served.")
>> >     private static final Arg<String> CLUSTER_NAME = Arg.create();
>> >
>> >     @NotNull
>> >     @NotEmpty
>> >     @CmdLine(name = "serverset_path", help = "ZooKeeper ServerSet path to
>> > register at.")
>> >     private static final Arg<String> SERVERSET_PATH = Arg.create();
>> >
>> > This makes it simple to add command line arguments.  However, it means
>> that
>> > a level of indirection is needed to parameterize and test the code
>> > consuming arg values.  We have various examples of this throughout the
>> > project.
>> >
>> > I propose that we change the way command line arguments are declared such
>> > that a Module with the above arguments would instead declare an interface
>> > that produces its parameters:
>> >
>> >     public interface Params {
>> >       @Help("Name to identify the cluster being served.")
>> >       String clusterName();
>> >
>> >       @NotEmpty
>> >       @Help("ZooKeeper ServerSet path to register at.")
>> >       String serversetPath();
>> >     }
>> >
>> >     public SchedulerModule(Params params) {
>> >       // Params are supplied to the module constructor.
>> >     }
>> >
>> > Please see this review for a complete example of this part of the change:
>> > https://reviews.apache.org/r/41804
>> >
>> > This is roughly the same amount of overhead for declaring arguments as
>> the
>> > current scenario, with the addition of a very obvious mechanism for
>> > swapping the source of parameters.  This allows us to isolate the body of
>> > code responsible for supplying configuration values, which we lack today.
>> >
>> > The remaining work is to bridge the gap between a command line argument
>> > system and Parameter interfaces.  This is relatively easy to do with
>> > dynamic proxies.  I have posted a proof of concept here:
>> > https://reviews.apache.org/r/42042
>> >
>> > Regarding (c), i have done some analysis of libraries available and i
>> > suggest argparse4j [3].  It has thorough documentation, no transitive
>> > dependencies, and is being actively developed (last release Dec 2015).
>> > However, i would like to emphasize that i think we should minimize
>> coupling
>> > to the argument parsing library so that we may switch in the future.
>> > Argparse4j has a feature that makes the non-critical feature (d)
>> possible.
>> >
>> > With that, what do you think?  Are there other goals we should add?  Does
>> > the plan make sense?
>> >
>> > [1]
>> >
>> https://github.com/apache/aurora/tree/master/commons-args/src/main/java/org/apache/aurora/common/args
>> > [2] https://github.com/twitter/commons
>> > [3] https://argparse4j.github.io/
>>

Mime
View raw message