aurora-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Zameer Manji <zma...@apache.org>
Subject Re: [PROPOSAL] Replace commons-args
Date Tue, 12 Jan 2016 17:55:30 GMT
I'm in favor of the entire proposal. I am also willing to do reviews for
this.

On Tue, Jan 12, 2016 at 9:22 AM, Joshua Cohen <jcohen@apache.org> wrote:

> I'm in favor of this proposal and will be very happy to see commons-args go
> away :).
>
> On Tue, Jan 12, 2016 at 6:53 AM, John Sirois <john@conductant.com> wrote:
>
> > 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
> >
>
> --
> Zameer Manji
>
>
>

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