aurora-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From John Sirois <j...@conductant.com>
Subject Re: [PROPOSAL] Replace commons-args
Date Tue, 12 Jan 2016 14:53:22 GMT
On Mon, Jan 11, 2016 at 9:49 PM, Bill Farner <wfarner@apache.org> wrote:

> Any other thoughts on this?  I will proceed with initial encapsulation work
> in the meantime, but am interested in any other pro/con opinions.
>

I've stayed quiet on this since Bill and I spoke about the idea offline in
its pre-seed phase.  I think the advantages have already been well-covered
so I'll point out the disadvantages I've seen (I implemented a system like
this in the past):

In the absence of the args system injecting args where they're needed (the
current Args does this and so is a 2nd form of data injection in addition
to guice), developers find it hard or inconvenient to plumb options from
the place where args are parsed (near main) to where they belong.  If DI is
not widely deployed - this can be a real problem.

That said, In Aurora we don't have this problem.  Guice is used extensively
for DI in aurora and it can get the options plumbed where we need them in
the standard way.

I'm very much in favor of this proposal.


> On Fri, Jan 8, 2016 at 10:25 AM, Maxim Khutornenko <maxim@apache.org>
> wrote:
>
> > "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/
> > >>
> >
>



-- 
John Sirois
303-512-3301

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message