Re: Review Request 41804: Proposal - shim interfaces to preface args system overhaul.

[Maxim Khutornenko <ma...@apache.org>]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41804/#review112643
-----------------------------------------------------------


Has the high level plan been discussed on dev list before or is it the first time this proposal is shaped up? It would be great to have an official design summary or at least a ticket capturing the goals of this refactoring.

- Maxim Khutornenko


On Dec. 31, 2015, 12:03 a.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41804/
> -----------------------------------------------------------
> 
> (Updated Dec. 31, 2015, 12:03 a.m.)
> 
> 
> Review request for Aurora, John Sirois and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> This begins to define a proposed replacement args API, from the perspective of the code consuming args.  Args will be defined in interfaces, which the eventual arg system will be responsible for implementing on the fly (mechanism TBD).  So while what is seen here is a large net increase in code, the eventual conclusion will be roughly equivalent in terms of lines of code in `Module`s.
> 
> There are a few goals with the replacement:
> - sidestep current development hurdles we have encountered with the args system (intellij/gradle not working nicely with apt)
> - leverage a well-maintained third-party argument parsing library
> - encourage better testability of Module classes by always injecting all args
> - enable user-friendly features like logical option groups for better help/usage output
> - stretch: enable alternative configuration inputs like a configuration file or environment variables
> 
> The rough plan of action is as follows (if the proposal looks good):
> 1. repeat this patch for all other `@CmdLine` declaration sites (28 files) 
> 2. introduce a 'boot' `Injector` that is loaded with bindings for these params implementations (i.e. centralize the `new ExecutorModuleParams() { .. }` boilerplate you see here
> 3. replace the args backend
>   (a) remove `@CmdLine Arg<>` declarations, moving help text to annotations on interface methods
>   (b) implement a system to inject proxies that implement params classes based on arg values, and binds them for injection
> 
> Given that this is a large multi-stage effort, i may opt to implement it on a branch and land it all at once in a stream of commits to avoid churn/confusion in the meantime.  Open to thoughts on that.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java 949c299bdbc54f976db994266fb97f3099256f13 
> 
> Diff: https://reviews.apache.org/r/41804/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 41804: Proposal - shim interfaces to preface args system overhaul.

[Bill Farner <wf...@apache.org>]

> On Jan. 4, 2016, 12:46 p.m., Maxim Khutornenko wrote:
> > Has the high level plan been discussed on dev list before or is it the first time this proposal is shaped up? It would be great to have an official design summary or at least a ticket capturing the goals of this refactoring.

Fair enough.  I always like to start with a straw man in code, so i'm still interested in your thoughts on what you see so far.


- Bill


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41804/#review112643
-----------------------------------------------------------


On Dec. 30, 2015, 4:03 p.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41804/
> -----------------------------------------------------------
> 
> (Updated Dec. 30, 2015, 4:03 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> This begins to define a proposed replacement args API, from the perspective of the code consuming args.  Args will be defined in interfaces, which the eventual arg system will be responsible for implementing on the fly (mechanism TBD).  So while what is seen here is a large net increase in code, the eventual conclusion will be roughly equivalent in terms of lines of code in `Module`s.
> 
> There are a few goals with the replacement:
> - sidestep current development hurdles we have encountered with the args system (intellij/gradle not working nicely with apt)
> - leverage a well-maintained third-party argument parsing library
> - encourage better testability of Module classes by always injecting all args
> - enable user-friendly features like logical option groups for better help/usage output
> - stretch: enable alternative configuration inputs like a configuration file or environment variables
> 
> The rough plan of action is as follows (if the proposal looks good):
> 1. repeat this patch for all other `@CmdLine` declaration sites (28 files) 
> 2. introduce a 'boot' `Injector` that is loaded with bindings for these params implementations (i.e. centralize the `new ExecutorModuleParams() { .. }` boilerplate you see here
> 3. replace the args backend
>   (a) remove `@CmdLine Arg<>` declarations, moving help text to annotations on interface methods
>   (b) implement a system to inject proxies that implement params classes based on arg values, and binds them for injection
> 
> Given that this is a large multi-stage effort, i may opt to implement it on a branch and land it all at once in a stream of commits to avoid churn/confusion in the meantime.  Open to thoughts on that.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java 949c299bdbc54f976db994266fb97f3099256f13 
> 
> Diff: https://reviews.apache.org/r/41804/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Bill Farner
> 
>