You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@aurora.apache.org by Bill Farner <wf...@apache.org> on 2016/01/08 02:18:05 UTC
[PROPOSAL] Replace commons-args
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/
Re: [PROPOSAL] Replace commons-args
Posted by Zameer Manji <zm...@apache.org>.
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 <jc...@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 <jo...@conductant.com> wrote:
>
> > On Mon, Jan 11, 2016 at 9:49 PM, Bill Farner <wf...@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 <ma...@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 <wf...@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 <wf...@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
>
>
>
Re: [PROPOSAL] Replace commons-args
Posted by Joshua Cohen <jc...@apache.org>.
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 <jo...@conductant.com> wrote:
> On Mon, Jan 11, 2016 at 9:49 PM, Bill Farner <wf...@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 <ma...@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 <wf...@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 <ma...@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 <wf...@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
>
Re: [PROPOSAL] Replace commons-args
Posted by John Sirois <jo...@conductant.com>.
On Mon, Jan 11, 2016 at 9:49 PM, Bill Farner <wf...@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 <ma...@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 <wf...@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 <ma...@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 <wf...@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
Re: [PROPOSAL] Replace commons-args
Posted by Bill Farner <wf...@apache.org>.
Any other thoughts on this? I will proceed with initial encapsulation work
in the meantime, but am interested in any other pro/con opinions.
On Fri, Jan 8, 2016 at 10:25 AM, Maxim Khutornenko <ma...@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 <wf...@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 <ma...@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 <wf...@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/
> >>
>
Re: [PROPOSAL] Replace commons-args
Posted by Maxim Khutornenko <ma...@apache.org>.
"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 <wf...@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 <ma...@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 <wf...@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/
>>
Re: [PROPOSAL] Replace commons-args
Posted by Bill Farner <wf...@apache.org>.
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 <ma...@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 <wf...@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/
>
Re: [PROPOSAL] Replace commons-args
Posted by Maxim Khutornenko <ma...@apache.org>.
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 <wf...@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/