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 <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/ > >> >
