aurora-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bill Farner <wfar...@apache.org>
Subject Re: [PROPOSAL] Replace commons-args
Date Fri, 08 Jan 2016 02:58:19 GMT
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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message