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 <jco...@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 <j...@conductant.com> wrote: > > > On Mon, Jan 11, 2016 at 9:49 PM, Bill Farner <wfar...@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 <wfar...@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 <wfar...@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 > > >
