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/
