aurora-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joshua Cohen <jco...@apache.org>
Subject Re: [PROPOSAL] Replace commons-args
Date Tue, 12 Jan 2016 17:22:27 GMT
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
>

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