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

Ship it!


I'd be happy with these commits landing as they come, but I'll recommend that 
be paired with a boilerplate comment referencing a tracking jira issue that 
explains the odd interfaces appearing during the transition.


src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java
 (line 122)
<https://reviews.apache.org/r/41804/#comment172832>

    It might be slightly less churn to move these in to the interface via 
default methods - when the cutover is made to the new system where default 
methods should only provide defaults, the edits will be line deletes or 
replacement with default values.


- John Sirois


On Dec. 30, 2015, 5: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, 5: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
> 
>

Reply via email to