Re: Review Request 62623: Use a simpler command line argument system

[Aurora ReviewBot]

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


Ship it!




Master (cb8e956) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Oct. 10, 2017, 11:16 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62623/
> ---
> 
> (Updated Oct. 10, 2017, 11:16 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This is a very big patch, lots of code removed.  I suggest starting with 
> `CliOptions.java` and `CommandLine.java` on page 5.  `CommandLIneTest` on 
> page 8 verifies that every registered option can be parsed.
> 
> I made a similar effort a while back, but was unable to finish due to time 
> constraints.  I'm now proposing a more drastic simplification in command line 
> argument handling in the scheduler.  Less magic, more approachable, less 
> brittle with other tools (gradle, IDEs).  The original approach made lots of 
> sense in the original environment where Aurora was developed (a monorepo in a 
> large organization), but this is no longer the case.
> 
> Historical context: 
> https://mail-archives.apache.org/mod_mbox/aurora-dev/201601.mbox/%3CCAFWq12VJPOxDViDP5OsmXU3skHR6rrp5Zp%2BXgi8HgeHic6pTJw%40mail.gmail.com%3E
> 
> 
> Diffs
> -
> 
>   build.gradle d7a4287a4a073aedb905af57309ed08f889eea0b 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 4da91591d325ab657dcd37325e5142c65db2ab8c 
>   commons-args/src/main/java/org/apache/aurora/common/args/ArgParser.java 
> 25ed25093bd9defd78df741b6f51e0de0d1f1709 
>   commons-args/src/main/java/org/apache/aurora/common/args/CmdLine.java 
> a72ea7a1669a53d282f9a5a3709add506c9d4b33 
>   commons-args/src/main/java/org/apache/aurora/common/args/NoParser.java 
> 3366531cebe6a79d9c568322f9117d7dbc3e824d 
>   commons-args/src/main/java/org/apache/aurora/common/args/Parser.java 
> 93c23234df38c9f917c0f582b51c25070f996c94 
>   commons-args/src/main/java/org/apache/aurora/common/args/ParserOracle.java 
> e84d3029a45cf7ea1f0bb885788c677ed649b60e 
>   commons-args/src/main/java/org/apache/aurora/common/args/Verifier.java 
> 8f09a9803b8a05b2ab11326d48f26cccaaa278d1 
>   commons-args/src/main/java/org/apache/aurora/common/args/VerifierFor.java 
> aad8807938f569d0dc7a970166ee71ea36f3537d 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/CmdLineProcessor.java
>  5fda5dc6cd8b6d97511073830278850d58bb0bc7 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/Configuration.java
>  1832d41feeafb07f4e7f0bef9dbcb6097e288508 
>   
> commons-args/src/main/resources/META-INF/services/javax.annotation.processing.Processor
>  b548fcdc4389af4420e57908a313435b5300445f 
>   commons/src/main/java/org/apache/aurora/common/args/ArgFilters.java 
> 9fd6eaec98516c3ca9b9a8647af848fcc96509bd 
>   commons/src/main/java/org/apache/aurora/common/args/ArgScanner.java 
> 6e7f23d79deff4983b7feb568320cb938583270b 
>   commons/src/main/java/org/apache/aurora/common/args/Args.java 
> 202835debce817df82bd3d860330d23d9710f489 
>   commons/src/main/java/org/apache/aurora/common/args/ArgumentInfo.java 
> a59d1091b36d0b7908143335cf49d0dafb6627a1 
>   commons/src/main/java/org/apache/aurora/common/args/OptionInfo.java 
> 2fcd3e8f068c4ef950aabe52ca8050dcbd562500 
>   commons/src/main/java/org/apache/aurora/common/args/Parsers.java 
> c4e5fafc49028bd15cfe1bd2a874e73792091cb6 
>   commons/src/main/java/org/apache/aurora/common/args/TypeUtil.java 
> 80cbdd00ce2abb298232793c5fd20c83c6ba6de4 
>   commons/src/main/java/org/apache/aurora/common/args/Verifiers.java 
> 0212873258867ccdb17244e7b0678a7346e79b73 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanExecute.java
>  a26b8a2049a16e96fd97ad2163556de41ba5686e 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanExecuteFileVerifier.java
>  5d9b36070af7c717af5edcc0eec6774c2e26c102 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanRead.java 
> 3fef6a98e4c926126ac93061dd3b32c39f131e3c 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanReadFileVerifier.java
>  8c26304733264b6670e56ab032d7b039f4dfdbfe 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanWrite.java 
> c2beeeb73d475ef9d03e1fde12e6d55983af81ed 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanWriteFileVerifier.java
>  eac273868d72993bc3e83742398c050db2ca4aea 
>   commons/src/main/java/org/apache/aurora/common/args/constraints/Exists.java 
> 

Re: Review Request 62623: Use a simpler command line argument system

[Aurora ReviewBot]

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



Master (cb8e956) is red with this patch.
  ./build-support/jenkins/build.sh

Caused by: java.lang.IllegalAccessException at CommandLineTest.java:431

org.apache.aurora.scheduler.config.CommandLineTest > testCustomOptions FAILED
java.lang.RuntimeException at CommandLineTest.java:76
Caused by: java.lang.IllegalAccessException at CommandLineTest.java:76
I1010 23:26:25.706 [ShutdownHook, SchedulerMain] Stopping scheduler services. 

1140 tests completed, 2 failed, 2 skipped
:test FAILED
:jacocoTestReport
Coverage report generated: 
file:///home/jenkins/jenkins-slave/workspace/AuroraBot/dist/reports/jacoco/test/html/index.html
:jacocoTestCoverageVerification[ant:jacocoReport] Rule violated for bundle 
aurora: branches covered ratio is 0.78, but expected minimum is 0.79
 FAILED

FAILURE: Build completed with 2 failures.

1: Task failed with an exception.
---
* What went wrong:
Execution failed for task ':test'.
> There were failing tests. See the report at: 
> file:///home/jenkins/jenkins-slave/workspace/AuroraBot/dist/reports/tests/test/index.html

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug 
option to get more log output.
==

2: Task failed with an exception.
---
* What went wrong:
Execution failed for task ':jacocoTestCoverageVerification'.
> Rule violated for bundle aurora: branches covered ratio is 0.78, but expected 
> minimum is 0.79

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug 
option to get more log output.
==

* Get more help at https://help.gradle.org

BUILD FAILED in 5m 55s
48 actionable tasks: 39 executed, 9 up-to-date


I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Oct. 10, 2017, 11:16 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62623/
> ---
> 
> (Updated Oct. 10, 2017, 11:16 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This is a very big patch, lots of code removed.  I suggest starting with 
> `CliOptions.java` and `CommandLine.java` on page 5.  `CommandLIneTest` on 
> page 8 verifies that every registered option can be parsed.
> 
> I made a similar effort a while back, but was unable to finish due to time 
> constraints.  I'm now proposing a more drastic simplification in command line 
> argument handling in the scheduler.  Less magic, more approachable, less 
> brittle with other tools (gradle, IDEs).  The original approach made lots of 
> sense in the original environment where Aurora was developed (a monorepo in a 
> large organization), but this is no longer the case.
> 
> Historical context: 
> https://mail-archives.apache.org/mod_mbox/aurora-dev/201601.mbox/%3CCAFWq12VJPOxDViDP5OsmXU3skHR6rrp5Zp%2BXgi8HgeHic6pTJw%40mail.gmail.com%3E
> 
> 
> Diffs
> -
> 
>   build.gradle d7a4287a4a073aedb905af57309ed08f889eea0b 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 4da91591d325ab657dcd37325e5142c65db2ab8c 
>   commons-args/src/main/java/org/apache/aurora/common/args/ArgParser.java 
> 25ed25093bd9defd78df741b6f51e0de0d1f1709 
>   commons-args/src/main/java/org/apache/aurora/common/args/CmdLine.java 
> a72ea7a1669a53d282f9a5a3709add506c9d4b33 
>   commons-args/src/main/java/org/apache/aurora/common/args/NoParser.java 
> 3366531cebe6a79d9c568322f9117d7dbc3e824d 
>   commons-args/src/main/java/org/apache/aurora/common/args/Parser.java 
> 93c23234df38c9f917c0f582b51c25070f996c94 
>   commons-args/src/main/java/org/apache/aurora/common/args/ParserOracle.java 
> e84d3029a45cf7ea1f0bb885788c677ed649b60e 
>   commons-args/src/main/java/org/apache/aurora/common/args/Verifier.java 
> 8f09a9803b8a05b2ab11326d48f26cccaaa278d1 
>   commons-args/src/main/java/org/apache/aurora/common/args/VerifierFor.java 
> aad8807938f569d0dc7a970166ee71ea36f3537d 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/CmdLineProcessor.java
>  5fda5dc6cd8b6d97511073830278850d58bb0bc7 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/Configuration.java
>  1832d41feeafb07f4e7f0bef9dbcb6097e288508 
>   
> commons-args/src/main/resources/META-INF/services/javax.annotation.processing.Processor
>  b548fcdc4389af4420e57908a313435b5300445f 
>   commons/src/main/java/org/apache/aurora/common/args/ArgFilters.java 
> 

Re: Review Request 62623: Use a simpler command line argument system

[Bill Farner]

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

(Updated Oct. 10, 2017, 4:16 p.m.)


Review request for Aurora, David McLaughlin and John Sirois.


Changes
---

make it easier to consume custom options


Repository: aurora


Description
---

This is a very big patch, lots of code removed.  I suggest starting with 
`CliOptions.java` and `CommandLine.java` on page 5.  `CommandLIneTest` on page 
8 verifies that every registered option can be parsed.

I made a similar effort a while back, but was unable to finish due to time 
constraints.  I'm now proposing a more drastic simplification in command line 
argument handling in the scheduler.  Less magic, more approachable, less 
brittle with other tools (gradle, IDEs).  The original approach made lots of 
sense in the original environment where Aurora was developed (a monorepo in a 
large organization), but this is no longer the case.

Historical context: 
https://mail-archives.apache.org/mod_mbox/aurora-dev/201601.mbox/%3CCAFWq12VJPOxDViDP5OsmXU3skHR6rrp5Zp%2BXgi8HgeHic6pTJw%40mail.gmail.com%3E


Diffs (updated)
-

  build.gradle d7a4287a4a073aedb905af57309ed08f889eea0b 
  commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
4da91591d325ab657dcd37325e5142c65db2ab8c 
  commons-args/src/main/java/org/apache/aurora/common/args/ArgParser.java 
25ed25093bd9defd78df741b6f51e0de0d1f1709 
  commons-args/src/main/java/org/apache/aurora/common/args/CmdLine.java 
a72ea7a1669a53d282f9a5a3709add506c9d4b33 
  commons-args/src/main/java/org/apache/aurora/common/args/NoParser.java 
3366531cebe6a79d9c568322f9117d7dbc3e824d 
  commons-args/src/main/java/org/apache/aurora/common/args/Parser.java 
93c23234df38c9f917c0f582b51c25070f996c94 
  commons-args/src/main/java/org/apache/aurora/common/args/ParserOracle.java 
e84d3029a45cf7ea1f0bb885788c677ed649b60e 
  commons-args/src/main/java/org/apache/aurora/common/args/Verifier.java 
8f09a9803b8a05b2ab11326d48f26cccaaa278d1 
  commons-args/src/main/java/org/apache/aurora/common/args/VerifierFor.java 
aad8807938f569d0dc7a970166ee71ea36f3537d 
  
commons-args/src/main/java/org/apache/aurora/common/args/apt/CmdLineProcessor.java
 5fda5dc6cd8b6d97511073830278850d58bb0bc7 
  
commons-args/src/main/java/org/apache/aurora/common/args/apt/Configuration.java 
1832d41feeafb07f4e7f0bef9dbcb6097e288508 
  
commons-args/src/main/resources/META-INF/services/javax.annotation.processing.Processor
 b548fcdc4389af4420e57908a313435b5300445f 
  commons/src/main/java/org/apache/aurora/common/args/ArgFilters.java 
9fd6eaec98516c3ca9b9a8647af848fcc96509bd 
  commons/src/main/java/org/apache/aurora/common/args/ArgScanner.java 
6e7f23d79deff4983b7feb568320cb938583270b 
  commons/src/main/java/org/apache/aurora/common/args/Args.java 
202835debce817df82bd3d860330d23d9710f489 
  commons/src/main/java/org/apache/aurora/common/args/ArgumentInfo.java 
a59d1091b36d0b7908143335cf49d0dafb6627a1 
  commons/src/main/java/org/apache/aurora/common/args/OptionInfo.java 
2fcd3e8f068c4ef950aabe52ca8050dcbd562500 
  commons/src/main/java/org/apache/aurora/common/args/Parsers.java 
c4e5fafc49028bd15cfe1bd2a874e73792091cb6 
  commons/src/main/java/org/apache/aurora/common/args/TypeUtil.java 
80cbdd00ce2abb298232793c5fd20c83c6ba6de4 
  commons/src/main/java/org/apache/aurora/common/args/Verifiers.java 
0212873258867ccdb17244e7b0678a7346e79b73 
  
commons/src/main/java/org/apache/aurora/common/args/constraints/CanExecute.java 
a26b8a2049a16e96fd97ad2163556de41ba5686e 
  
commons/src/main/java/org/apache/aurora/common/args/constraints/CanExecuteFileVerifier.java
 5d9b36070af7c717af5edcc0eec6774c2e26c102 
  commons/src/main/java/org/apache/aurora/common/args/constraints/CanRead.java 
3fef6a98e4c926126ac93061dd3b32c39f131e3c 
  
commons/src/main/java/org/apache/aurora/common/args/constraints/CanReadFileVerifier.java
 8c26304733264b6670e56ab032d7b039f4dfdbfe 
  commons/src/main/java/org/apache/aurora/common/args/constraints/CanWrite.java 
c2beeeb73d475ef9d03e1fde12e6d55983af81ed 
  
commons/src/main/java/org/apache/aurora/common/args/constraints/CanWriteFileVerifier.java
 eac273868d72993bc3e83742398c050db2ca4aea 
  commons/src/main/java/org/apache/aurora/common/args/constraints/Exists.java 
217d10e561b175885d6bb2a058225935fd88b37a 
  
commons/src/main/java/org/apache/aurora/common/args/constraints/ExistsFileVerifier.java
 e79f54701dea77440b585034790cc4cee987b99a 
  
commons/src/main/java/org/apache/aurora/common/args/constraints/IsDirectory.java
 d909994937e5ab7f9e125bdf000c8a22c2f84734 
  
commons/src/main/java/org/apache/aurora/common/args/constraints/IsDirectoryFileVerifier.java
 968a098357ad37cd6d6121fc049990ffea0fb2d8 
  commons/src/main/java/org/apache/aurora/common/args/constraints/NotEmpty.java 
8ff96afdfa2b837526956a77c8819c097c14881c 
  

Re: Review Request 62623: Use a simpler command line argument system

[David McLaughlin]

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


Ship it!




Ship It!

- David McLaughlin


On Oct. 10, 2017, 2:37 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62623/
> ---
> 
> (Updated Oct. 10, 2017, 2:37 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This is a very big patch, lots of code removed.  I suggest starting with 
> `CliOptions.java` and `CommandLine.java` on page 5.  `CommandLIneTest` on 
> page 8 verifies that every registered option can be parsed.
> 
> I made a similar effort a while back, but was unable to finish due to time 
> constraints.  I'm now proposing a more drastic simplification in command line 
> argument handling in the scheduler.  Less magic, more approachable, less 
> brittle with other tools (gradle, IDEs).  The original approach made lots of 
> sense in the original environment where Aurora was developed (a monorepo in a 
> large organization), but this is no longer the case.
> 
> Historical context: 
> https://mail-archives.apache.org/mod_mbox/aurora-dev/201601.mbox/%3CCAFWq12VJPOxDViDP5OsmXU3skHR6rrp5Zp%2BXgi8HgeHic6pTJw%40mail.gmail.com%3E
> 
> 
> Diffs
> -
> 
>   build.gradle d7a4287a4a073aedb905af57309ed08f889eea0b 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 4da91591d325ab657dcd37325e5142c65db2ab8c 
>   commons-args/src/main/java/org/apache/aurora/common/args/ArgParser.java 
> 25ed25093bd9defd78df741b6f51e0de0d1f1709 
>   commons-args/src/main/java/org/apache/aurora/common/args/CmdLine.java 
> a72ea7a1669a53d282f9a5a3709add506c9d4b33 
>   commons-args/src/main/java/org/apache/aurora/common/args/NoParser.java 
> 3366531cebe6a79d9c568322f9117d7dbc3e824d 
>   commons-args/src/main/java/org/apache/aurora/common/args/Parser.java 
> 93c23234df38c9f917c0f582b51c25070f996c94 
>   commons-args/src/main/java/org/apache/aurora/common/args/ParserOracle.java 
> e84d3029a45cf7ea1f0bb885788c677ed649b60e 
>   commons-args/src/main/java/org/apache/aurora/common/args/Verifier.java 
> 8f09a9803b8a05b2ab11326d48f26cccaaa278d1 
>   commons-args/src/main/java/org/apache/aurora/common/args/VerifierFor.java 
> aad8807938f569d0dc7a970166ee71ea36f3537d 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/CmdLineProcessor.java
>  5fda5dc6cd8b6d97511073830278850d58bb0bc7 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/Configuration.java
>  1832d41feeafb07f4e7f0bef9dbcb6097e288508 
>   
> commons-args/src/main/resources/META-INF/services/javax.annotation.processing.Processor
>  b548fcdc4389af4420e57908a313435b5300445f 
>   commons/src/main/java/org/apache/aurora/common/args/ArgFilters.java 
> 9fd6eaec98516c3ca9b9a8647af848fcc96509bd 
>   commons/src/main/java/org/apache/aurora/common/args/ArgScanner.java 
> 6e7f23d79deff4983b7feb568320cb938583270b 
>   commons/src/main/java/org/apache/aurora/common/args/Args.java 
> 202835debce817df82bd3d860330d23d9710f489 
>   commons/src/main/java/org/apache/aurora/common/args/ArgumentInfo.java 
> a59d1091b36d0b7908143335cf49d0dafb6627a1 
>   commons/src/main/java/org/apache/aurora/common/args/OptionInfo.java 
> 2fcd3e8f068c4ef950aabe52ca8050dcbd562500 
>   commons/src/main/java/org/apache/aurora/common/args/Parsers.java 
> c4e5fafc49028bd15cfe1bd2a874e73792091cb6 
>   commons/src/main/java/org/apache/aurora/common/args/TypeUtil.java 
> 80cbdd00ce2abb298232793c5fd20c83c6ba6de4 
>   commons/src/main/java/org/apache/aurora/common/args/Verifiers.java 
> 0212873258867ccdb17244e7b0678a7346e79b73 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanExecute.java
>  a26b8a2049a16e96fd97ad2163556de41ba5686e 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanExecuteFileVerifier.java
>  5d9b36070af7c717af5edcc0eec6774c2e26c102 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanRead.java 
> 3fef6a98e4c926126ac93061dd3b32c39f131e3c 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanReadFileVerifier.java
>  8c26304733264b6670e56ab032d7b039f4dfdbfe 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanWrite.java 
> c2beeeb73d475ef9d03e1fde12e6d55983af81ed 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanWriteFileVerifier.java
>  eac273868d72993bc3e83742398c050db2ca4aea 
>   commons/src/main/java/org/apache/aurora/common/args/constraints/Exists.java 
> 217d10e561b175885d6bb2a058225935fd88b37a 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/ExistsFileVerifier.java
>  

Re: Review Request 62623: Use a simpler command line argument system

[Aurora ReviewBot]

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


Ship it!




Master (0169b81) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Oct. 10, 2017, 2:37 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62623/
> ---
> 
> (Updated Oct. 10, 2017, 2:37 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This is a very big patch, lots of code removed.  I suggest starting with 
> `CliOptions.java` and `CommandLine.java` on page 5.  `CommandLIneTest` on 
> page 8 verifies that every registered option can be parsed.
> 
> I made a similar effort a while back, but was unable to finish due to time 
> constraints.  I'm now proposing a more drastic simplification in command line 
> argument handling in the scheduler.  Less magic, more approachable, less 
> brittle with other tools (gradle, IDEs).  The original approach made lots of 
> sense in the original environment where Aurora was developed (a monorepo in a 
> large organization), but this is no longer the case.
> 
> Historical context: 
> https://mail-archives.apache.org/mod_mbox/aurora-dev/201601.mbox/%3CCAFWq12VJPOxDViDP5OsmXU3skHR6rrp5Zp%2BXgi8HgeHic6pTJw%40mail.gmail.com%3E
> 
> 
> Diffs
> -
> 
>   build.gradle d7a4287a4a073aedb905af57309ed08f889eea0b 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 4da91591d325ab657dcd37325e5142c65db2ab8c 
>   commons-args/src/main/java/org/apache/aurora/common/args/ArgParser.java 
> 25ed25093bd9defd78df741b6f51e0de0d1f1709 
>   commons-args/src/main/java/org/apache/aurora/common/args/CmdLine.java 
> a72ea7a1669a53d282f9a5a3709add506c9d4b33 
>   commons-args/src/main/java/org/apache/aurora/common/args/NoParser.java 
> 3366531cebe6a79d9c568322f9117d7dbc3e824d 
>   commons-args/src/main/java/org/apache/aurora/common/args/Parser.java 
> 93c23234df38c9f917c0f582b51c25070f996c94 
>   commons-args/src/main/java/org/apache/aurora/common/args/ParserOracle.java 
> e84d3029a45cf7ea1f0bb885788c677ed649b60e 
>   commons-args/src/main/java/org/apache/aurora/common/args/Verifier.java 
> 8f09a9803b8a05b2ab11326d48f26cccaaa278d1 
>   commons-args/src/main/java/org/apache/aurora/common/args/VerifierFor.java 
> aad8807938f569d0dc7a970166ee71ea36f3537d 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/CmdLineProcessor.java
>  5fda5dc6cd8b6d97511073830278850d58bb0bc7 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/Configuration.java
>  1832d41feeafb07f4e7f0bef9dbcb6097e288508 
>   
> commons-args/src/main/resources/META-INF/services/javax.annotation.processing.Processor
>  b548fcdc4389af4420e57908a313435b5300445f 
>   commons/src/main/java/org/apache/aurora/common/args/ArgFilters.java 
> 9fd6eaec98516c3ca9b9a8647af848fcc96509bd 
>   commons/src/main/java/org/apache/aurora/common/args/ArgScanner.java 
> 6e7f23d79deff4983b7feb568320cb938583270b 
>   commons/src/main/java/org/apache/aurora/common/args/Args.java 
> 202835debce817df82bd3d860330d23d9710f489 
>   commons/src/main/java/org/apache/aurora/common/args/ArgumentInfo.java 
> a59d1091b36d0b7908143335cf49d0dafb6627a1 
>   commons/src/main/java/org/apache/aurora/common/args/OptionInfo.java 
> 2fcd3e8f068c4ef950aabe52ca8050dcbd562500 
>   commons/src/main/java/org/apache/aurora/common/args/Parsers.java 
> c4e5fafc49028bd15cfe1bd2a874e73792091cb6 
>   commons/src/main/java/org/apache/aurora/common/args/TypeUtil.java 
> 80cbdd00ce2abb298232793c5fd20c83c6ba6de4 
>   commons/src/main/java/org/apache/aurora/common/args/Verifiers.java 
> 0212873258867ccdb17244e7b0678a7346e79b73 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanExecute.java
>  a26b8a2049a16e96fd97ad2163556de41ba5686e 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanExecuteFileVerifier.java
>  5d9b36070af7c717af5edcc0eec6774c2e26c102 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanRead.java 
> 3fef6a98e4c926126ac93061dd3b32c39f131e3c 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanReadFileVerifier.java
>  8c26304733264b6670e56ab032d7b039f4dfdbfe 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanWrite.java 
> c2beeeb73d475ef9d03e1fde12e6d55983af81ed 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanWriteFileVerifier.java
>  eac273868d72993bc3e83742398c050db2ca4aea 
>   commons/src/main/java/org/apache/aurora/common/args/constraints/Exists.java 
> 

Re: Review Request 62623: Use a simpler command line argument system

[Aurora ReviewBot]

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



Master (0169b81) is red with this patch.
  ./build-support/jenkins/build.sh

:licenseTest UP-TO-DATE
:license UP-TO-DATE
:pmdJmh
:pmdMain
:pmdTest
:testI1010 15:31:35.753 [ShutdownHook, SchedulerMain] Stopping scheduler 
services. 
 FAILED
:jacocoTestReport
Coverage report generated: 
file:///home/jenkins/jenkins-slave/workspace/AuroraBot/dist/reports/jacoco/test/html/index.html
:jacocoTestCoverageVerification[ant:jacocoReport] Rule violated for bundle 
aurora: instructions covered ratio is 0.81, but expected minimum is 0.87
[ant:jacocoReport] Rule violated for bundle aurora: branches covered ratio is 
0.69, but expected minimum is 0.79
 FAILED

FAILURE: Build completed with 2 failures.

1: Task failed with an exception.
---
* What went wrong:
Execution failed for task ':test'.
> Process 'Gradle Test Executor 8' finished with non-zero exit value 137

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug 
option to get more log output.
==

2: Task failed with an exception.
---
* What went wrong:
Execution failed for task ':jacocoTestCoverageVerification'.
> Rule violated for bundle aurora: instructions covered ratio is 0.81, but 
> expected minimum is 0.87
  Rule violated for bundle aurora: branches covered ratio is 0.69, but expected 
minimum is 0.79

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug 
option to get more log output.
==

* Get more help at https://help.gradle.org

BUILD FAILED in 20m 23s
48 actionable tasks: 39 executed, 9 up-to-date


I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Oct. 10, 2017, 2:37 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62623/
> ---
> 
> (Updated Oct. 10, 2017, 2:37 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This is a very big patch, lots of code removed.  I suggest starting with 
> `CliOptions.java` and `CommandLine.java` on page 5.  `CommandLIneTest` on 
> page 8 verifies that every registered option can be parsed.
> 
> I made a similar effort a while back, but was unable to finish due to time 
> constraints.  I'm now proposing a more drastic simplification in command line 
> argument handling in the scheduler.  Less magic, more approachable, less 
> brittle with other tools (gradle, IDEs).  The original approach made lots of 
> sense in the original environment where Aurora was developed (a monorepo in a 
> large organization), but this is no longer the case.
> 
> Historical context: 
> https://mail-archives.apache.org/mod_mbox/aurora-dev/201601.mbox/%3CCAFWq12VJPOxDViDP5OsmXU3skHR6rrp5Zp%2BXgi8HgeHic6pTJw%40mail.gmail.com%3E
> 
> 
> Diffs
> -
> 
>   build.gradle d7a4287a4a073aedb905af57309ed08f889eea0b 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 4da91591d325ab657dcd37325e5142c65db2ab8c 
>   commons-args/src/main/java/org/apache/aurora/common/args/ArgParser.java 
> 25ed25093bd9defd78df741b6f51e0de0d1f1709 
>   commons-args/src/main/java/org/apache/aurora/common/args/CmdLine.java 
> a72ea7a1669a53d282f9a5a3709add506c9d4b33 
>   commons-args/src/main/java/org/apache/aurora/common/args/NoParser.java 
> 3366531cebe6a79d9c568322f9117d7dbc3e824d 
>   commons-args/src/main/java/org/apache/aurora/common/args/Parser.java 
> 93c23234df38c9f917c0f582b51c25070f996c94 
>   commons-args/src/main/java/org/apache/aurora/common/args/ParserOracle.java 
> e84d3029a45cf7ea1f0bb885788c677ed649b60e 
>   commons-args/src/main/java/org/apache/aurora/common/args/Verifier.java 
> 8f09a9803b8a05b2ab11326d48f26cccaaa278d1 
>   commons-args/src/main/java/org/apache/aurora/common/args/VerifierFor.java 
> aad8807938f569d0dc7a970166ee71ea36f3537d 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/CmdLineProcessor.java
>  5fda5dc6cd8b6d97511073830278850d58bb0bc7 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/Configuration.java
>  1832d41feeafb07f4e7f0bef9dbcb6097e288508 
>   
> commons-args/src/main/resources/META-INF/services/javax.annotation.processing.Processor
>  b548fcdc4389af4420e57908a313435b5300445f 
>   commons/src/main/java/org/apache/aurora/common/args/ArgFilters.java 
> 9fd6eaec98516c3ca9b9a8647af848fcc96509bd 
>   commons/src/main/java/org/apache/aurora/common/args/ArgScanner.java 
> 

Re: Review Request 62623: Use a simpler command line argument system

[Aurora ReviewBot]

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



Master (0169b81) is red with this patch.
  ./build-support/jenkins/build.sh

#
# There is insufficient memory for the Java Runtime Environment to continue.
# Native memory allocation (mmap) failed to map 232783872 bytes for committing 
reserved memory.
# An error report file with more information is saved as:
# /home/jenkins/jenkins-slave/workspace/AuroraBot/hs_err_pid31468.log
I1010 14:49:58.885 [ShutdownHook, SchedulerMain] Stopping scheduler services. 
:test FAILED
:jacocoTestReport
Coverage report generated: 
file:///home/jenkins/jenkins-slave/workspace/AuroraBot/dist/reports/jacoco/test/html/index.html
:jacocoTestCoverageVerification[ant:jacocoReport] Rule violated for bundle 
aurora: instructions covered ratio is 0.80, but expected minimum is 0.87
[ant:jacocoReport] Rule violated for bundle aurora: branches covered ratio is 
0.69, but expected minimum is 0.79
 FAILED

FAILURE: Build completed with 2 failures.

1: Task failed with an exception.
---
* What went wrong:
Execution failed for task ':test'.
> Process 'Gradle Test Executor 8' finished with non-zero exit value 1

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug 
option to get more log output.
==

2: Task failed with an exception.
---
* What went wrong:
Execution failed for task ':jacocoTestCoverageVerification'.
> Rule violated for bundle aurora: instructions covered ratio is 0.80, but 
> expected minimum is 0.87
  Rule violated for bundle aurora: branches covered ratio is 0.69, but expected 
minimum is 0.79

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug 
option to get more log output.
==

* Get more help at https://help.gradle.org

BUILD FAILED in 7m 28s
48 actionable tasks: 39 executed, 9 up-to-date


I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Oct. 10, 2017, 2:37 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62623/
> ---
> 
> (Updated Oct. 10, 2017, 2:37 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This is a very big patch, lots of code removed.  I suggest starting with 
> `CliOptions.java` and `CommandLine.java` on page 5.  `CommandLIneTest` on 
> page 8 verifies that every registered option can be parsed.
> 
> I made a similar effort a while back, but was unable to finish due to time 
> constraints.  I'm now proposing a more drastic simplification in command line 
> argument handling in the scheduler.  Less magic, more approachable, less 
> brittle with other tools (gradle, IDEs).  The original approach made lots of 
> sense in the original environment where Aurora was developed (a monorepo in a 
> large organization), but this is no longer the case.
> 
> Historical context: 
> https://mail-archives.apache.org/mod_mbox/aurora-dev/201601.mbox/%3CCAFWq12VJPOxDViDP5OsmXU3skHR6rrp5Zp%2BXgi8HgeHic6pTJw%40mail.gmail.com%3E
> 
> 
> Diffs
> -
> 
>   build.gradle d7a4287a4a073aedb905af57309ed08f889eea0b 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 4da91591d325ab657dcd37325e5142c65db2ab8c 
>   commons-args/src/main/java/org/apache/aurora/common/args/ArgParser.java 
> 25ed25093bd9defd78df741b6f51e0de0d1f1709 
>   commons-args/src/main/java/org/apache/aurora/common/args/CmdLine.java 
> a72ea7a1669a53d282f9a5a3709add506c9d4b33 
>   commons-args/src/main/java/org/apache/aurora/common/args/NoParser.java 
> 3366531cebe6a79d9c568322f9117d7dbc3e824d 
>   commons-args/src/main/java/org/apache/aurora/common/args/Parser.java 
> 93c23234df38c9f917c0f582b51c25070f996c94 
>   commons-args/src/main/java/org/apache/aurora/common/args/ParserOracle.java 
> e84d3029a45cf7ea1f0bb885788c677ed649b60e 
>   commons-args/src/main/java/org/apache/aurora/common/args/Verifier.java 
> 8f09a9803b8a05b2ab11326d48f26cccaaa278d1 
>   commons-args/src/main/java/org/apache/aurora/common/args/VerifierFor.java 
> aad8807938f569d0dc7a970166ee71ea36f3537d 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/CmdLineProcessor.java
>  5fda5dc6cd8b6d97511073830278850d58bb0bc7 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/Configuration.java
>  1832d41feeafb07f4e7f0bef9dbcb6097e288508 
>   
> commons-args/src/main/resources/META-INF/services/javax.annotation.processing.Processor
>  

Re: Review Request 62623: Use a simpler command line argument system

[Bill Farner]

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

(Updated Oct. 10, 2017, 7:37 a.m.)


Review request for Aurora, David McLaughlin and John Sirois.


Changes
---

- added and/or ported converters where needed
- added option validators
- added a comprehensive test


Repository: aurora


Description (updated)
---

This is a very big patch, lots of code removed.  I suggest starting with 
`CliOptions.java` and `CommandLine.java` on page 5.  `CommandLIneTest` on page 
8 verifies that every registered option can be parsed.

I made a similar effort a while back, but was unable to finish due to time 
constraints.  I'm now proposing a more drastic simplification in command line 
argument handling in the scheduler.  Less magic, more approachable, less 
brittle with other tools (gradle, IDEs).  The original approach made lots of 
sense in the original environment where Aurora was developed (a monorepo in a 
large organization), but this is no longer the case.

Historical context: 
https://mail-archives.apache.org/mod_mbox/aurora-dev/201601.mbox/%3CCAFWq12VJPOxDViDP5OsmXU3skHR6rrp5Zp%2BXgi8HgeHic6pTJw%40mail.gmail.com%3E


Diffs (updated)
-

  build.gradle d7a4287a4a073aedb905af57309ed08f889eea0b 
  commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
4da91591d325ab657dcd37325e5142c65db2ab8c 
  commons-args/src/main/java/org/apache/aurora/common/args/ArgParser.java 
25ed25093bd9defd78df741b6f51e0de0d1f1709 
  commons-args/src/main/java/org/apache/aurora/common/args/CmdLine.java 
a72ea7a1669a53d282f9a5a3709add506c9d4b33 
  commons-args/src/main/java/org/apache/aurora/common/args/NoParser.java 
3366531cebe6a79d9c568322f9117d7dbc3e824d 
  commons-args/src/main/java/org/apache/aurora/common/args/Parser.java 
93c23234df38c9f917c0f582b51c25070f996c94 
  commons-args/src/main/java/org/apache/aurora/common/args/ParserOracle.java 
e84d3029a45cf7ea1f0bb885788c677ed649b60e 
  commons-args/src/main/java/org/apache/aurora/common/args/Verifier.java 
8f09a9803b8a05b2ab11326d48f26cccaaa278d1 
  commons-args/src/main/java/org/apache/aurora/common/args/VerifierFor.java 
aad8807938f569d0dc7a970166ee71ea36f3537d 
  
commons-args/src/main/java/org/apache/aurora/common/args/apt/CmdLineProcessor.java
 5fda5dc6cd8b6d97511073830278850d58bb0bc7 
  
commons-args/src/main/java/org/apache/aurora/common/args/apt/Configuration.java 
1832d41feeafb07f4e7f0bef9dbcb6097e288508 
  
commons-args/src/main/resources/META-INF/services/javax.annotation.processing.Processor
 b548fcdc4389af4420e57908a313435b5300445f 
  commons/src/main/java/org/apache/aurora/common/args/ArgFilters.java 
9fd6eaec98516c3ca9b9a8647af848fcc96509bd 
  commons/src/main/java/org/apache/aurora/common/args/ArgScanner.java 
6e7f23d79deff4983b7feb568320cb938583270b 
  commons/src/main/java/org/apache/aurora/common/args/Args.java 
202835debce817df82bd3d860330d23d9710f489 
  commons/src/main/java/org/apache/aurora/common/args/ArgumentInfo.java 
a59d1091b36d0b7908143335cf49d0dafb6627a1 
  commons/src/main/java/org/apache/aurora/common/args/OptionInfo.java 
2fcd3e8f068c4ef950aabe52ca8050dcbd562500 
  commons/src/main/java/org/apache/aurora/common/args/Parsers.java 
c4e5fafc49028bd15cfe1bd2a874e73792091cb6 
  commons/src/main/java/org/apache/aurora/common/args/TypeUtil.java 
80cbdd00ce2abb298232793c5fd20c83c6ba6de4 
  commons/src/main/java/org/apache/aurora/common/args/Verifiers.java 
0212873258867ccdb17244e7b0678a7346e79b73 
  
commons/src/main/java/org/apache/aurora/common/args/constraints/CanExecute.java 
a26b8a2049a16e96fd97ad2163556de41ba5686e 
  
commons/src/main/java/org/apache/aurora/common/args/constraints/CanExecuteFileVerifier.java
 5d9b36070af7c717af5edcc0eec6774c2e26c102 
  commons/src/main/java/org/apache/aurora/common/args/constraints/CanRead.java 
3fef6a98e4c926126ac93061dd3b32c39f131e3c 
  
commons/src/main/java/org/apache/aurora/common/args/constraints/CanReadFileVerifier.java
 8c26304733264b6670e56ab032d7b039f4dfdbfe 
  commons/src/main/java/org/apache/aurora/common/args/constraints/CanWrite.java 
c2beeeb73d475ef9d03e1fde12e6d55983af81ed 
  
commons/src/main/java/org/apache/aurora/common/args/constraints/CanWriteFileVerifier.java
 eac273868d72993bc3e83742398c050db2ca4aea 
  commons/src/main/java/org/apache/aurora/common/args/constraints/Exists.java 
217d10e561b175885d6bb2a058225935fd88b37a 
  
commons/src/main/java/org/apache/aurora/common/args/constraints/ExistsFileVerifier.java
 e79f54701dea77440b585034790cc4cee987b99a 
  
commons/src/main/java/org/apache/aurora/common/args/constraints/IsDirectory.java
 d909994937e5ab7f9e125bdf000c8a22c2f84734 
  
commons/src/main/java/org/apache/aurora/common/args/constraints/IsDirectoryFileVerifier.java
 968a098357ad37cd6d6121fc049990ffea0fb2d8 
  commons/src/main/java/org/apache/aurora/common/args/constraints/NotEmpty.java 

Re: Review Request 62623: Use a simpler command line argument system

[Bill Farner]

> On Oct. 8, 2017, 10:08 a.m., Stephan Erb wrote:
> > Is anything blocking the merge of this patch? I need to add a new flag and 
> > I am wondering if I should sit out until this one is merged :)

Go ahead with your patch, i'll deal with the merge conflict.

The only remaining area for me to address is option type safety.  jcommander 
only reports lack of support for an option type when parsing a value from a 
string rather than when collecting option declarations.  This makes it more 
difficult to have confidence that all declared option types have registered 
converters.  This is furthermore problematic for `List` and `Set` options, for 
which jcommander will silently add `String`s if a matching converter is not 
found.


- Bill


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


On Sept. 29, 2017, 7:07 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62623/
> ---
> 
> (Updated Sept. 29, 2017, 7:07 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> **NOTE: this patch is not ready to commit, but is ready for initial 
> discussion on the direction**
> 
> This is a very big patch, lots of code removed.  I suggest starting with 
> `CliOptions.java` on page 5, then looking at a few module classes to see the 
> call site changes.
> 
> I made a similar effort a while back, but was unable to finish due to time 
> constraints.  I'm now proposing a more drastic simplification in command line 
> argument handling in the scheduler.  Less magic, more approachable, less 
> brittle with other tools (gradle, IDEs).  The original approach made lots of 
> sense in the original environment where Aurora was developed (a monorepo in a 
> large organization), but this is no longer the case.
> 
> Historical context: 
> https://mail-archives.apache.org/mod_mbox/aurora-dev/201601.mbox/%3CCAFWq12VJPOxDViDP5OsmXU3skHR6rrp5Zp%2BXgi8HgeHic6pTJw%40mail.gmail.com%3E
> 
> 
> Diffs
> -
> 
>   build.gradle f9579a38de95ff9e11cf2e44980d01e103226389 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 4da91591d325ab657dcd37325e5142c65db2ab8c 
>   commons-args/src/main/java/org/apache/aurora/common/args/ArgParser.java 
> 25ed25093bd9defd78df741b6f51e0de0d1f1709 
>   commons-args/src/main/java/org/apache/aurora/common/args/CmdLine.java 
> a72ea7a1669a53d282f9a5a3709add506c9d4b33 
>   commons-args/src/main/java/org/apache/aurora/common/args/NoParser.java 
> 3366531cebe6a79d9c568322f9117d7dbc3e824d 
>   commons-args/src/main/java/org/apache/aurora/common/args/Parser.java 
> 93c23234df38c9f917c0f582b51c25070f996c94 
>   commons-args/src/main/java/org/apache/aurora/common/args/ParserOracle.java 
> e84d3029a45cf7ea1f0bb885788c677ed649b60e 
>   commons-args/src/main/java/org/apache/aurora/common/args/Verifier.java 
> 8f09a9803b8a05b2ab11326d48f26cccaaa278d1 
>   commons-args/src/main/java/org/apache/aurora/common/args/VerifierFor.java 
> aad8807938f569d0dc7a970166ee71ea36f3537d 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/CmdLineProcessor.java
>  5fda5dc6cd8b6d97511073830278850d58bb0bc7 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/Configuration.java
>  1832d41feeafb07f4e7f0bef9dbcb6097e288508 
>   
> commons-args/src/main/resources/META-INF/services/javax.annotation.processing.Processor
>  b548fcdc4389af4420e57908a313435b5300445f 
>   commons/src/main/java/org/apache/aurora/common/args/ArgFilters.java 
> 9fd6eaec98516c3ca9b9a8647af848fcc96509bd 
>   commons/src/main/java/org/apache/aurora/common/args/ArgScanner.java 
> 6e7f23d79deff4983b7feb568320cb938583270b 
>   commons/src/main/java/org/apache/aurora/common/args/Args.java 
> 202835debce817df82bd3d860330d23d9710f489 
>   commons/src/main/java/org/apache/aurora/common/args/ArgumentInfo.java 
> a59d1091b36d0b7908143335cf49d0dafb6627a1 
>   commons/src/main/java/org/apache/aurora/common/args/OptionInfo.java 
> 2fcd3e8f068c4ef950aabe52ca8050dcbd562500 
>   commons/src/main/java/org/apache/aurora/common/args/Parsers.java 
> c4e5fafc49028bd15cfe1bd2a874e73792091cb6 
>   commons/src/main/java/org/apache/aurora/common/args/TypeUtil.java 
> 80cbdd00ce2abb298232793c5fd20c83c6ba6de4 
>   commons/src/main/java/org/apache/aurora/common/args/Verifiers.java 
> 0212873258867ccdb17244e7b0678a7346e79b73 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanExecute.java
>  a26b8a2049a16e96fd97ad2163556de41ba5686e 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanExecuteFileVerifier.java
>  5d9b36070af7c717af5edcc0eec6774c2e26c102 
>   
> 

Re: Review Request 62623: Use a simpler command line argument system

[Stephan Erb]

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



Is anything blocking the merge of this patch? I need to add a new flag and I am 
wondering if I should sit out until this one is merged :)

- Stephan Erb


On Sept. 30, 2017, 4:07 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62623/
> ---
> 
> (Updated Sept. 30, 2017, 4:07 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> **NOTE: this patch is not ready to commit, but is ready for initial 
> discussion on the direction**
> 
> This is a very big patch, lots of code removed.  I suggest starting with 
> `CliOptions.java` on page 5, then looking at a few module classes to see the 
> call site changes.
> 
> I made a similar effort a while back, but was unable to finish due to time 
> constraints.  I'm now proposing a more drastic simplification in command line 
> argument handling in the scheduler.  Less magic, more approachable, less 
> brittle with other tools (gradle, IDEs).  The original approach made lots of 
> sense in the original environment where Aurora was developed (a monorepo in a 
> large organization), but this is no longer the case.
> 
> Historical context: 
> https://mail-archives.apache.org/mod_mbox/aurora-dev/201601.mbox/%3CCAFWq12VJPOxDViDP5OsmXU3skHR6rrp5Zp%2BXgi8HgeHic6pTJw%40mail.gmail.com%3E
> 
> 
> Diffs
> -
> 
>   build.gradle f9579a38de95ff9e11cf2e44980d01e103226389 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 4da91591d325ab657dcd37325e5142c65db2ab8c 
>   commons-args/src/main/java/org/apache/aurora/common/args/ArgParser.java 
> 25ed25093bd9defd78df741b6f51e0de0d1f1709 
>   commons-args/src/main/java/org/apache/aurora/common/args/CmdLine.java 
> a72ea7a1669a53d282f9a5a3709add506c9d4b33 
>   commons-args/src/main/java/org/apache/aurora/common/args/NoParser.java 
> 3366531cebe6a79d9c568322f9117d7dbc3e824d 
>   commons-args/src/main/java/org/apache/aurora/common/args/Parser.java 
> 93c23234df38c9f917c0f582b51c25070f996c94 
>   commons-args/src/main/java/org/apache/aurora/common/args/ParserOracle.java 
> e84d3029a45cf7ea1f0bb885788c677ed649b60e 
>   commons-args/src/main/java/org/apache/aurora/common/args/Verifier.java 
> 8f09a9803b8a05b2ab11326d48f26cccaaa278d1 
>   commons-args/src/main/java/org/apache/aurora/common/args/VerifierFor.java 
> aad8807938f569d0dc7a970166ee71ea36f3537d 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/CmdLineProcessor.java
>  5fda5dc6cd8b6d97511073830278850d58bb0bc7 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/Configuration.java
>  1832d41feeafb07f4e7f0bef9dbcb6097e288508 
>   
> commons-args/src/main/resources/META-INF/services/javax.annotation.processing.Processor
>  b548fcdc4389af4420e57908a313435b5300445f 
>   commons/src/main/java/org/apache/aurora/common/args/ArgFilters.java 
> 9fd6eaec98516c3ca9b9a8647af848fcc96509bd 
>   commons/src/main/java/org/apache/aurora/common/args/ArgScanner.java 
> 6e7f23d79deff4983b7feb568320cb938583270b 
>   commons/src/main/java/org/apache/aurora/common/args/Args.java 
> 202835debce817df82bd3d860330d23d9710f489 
>   commons/src/main/java/org/apache/aurora/common/args/ArgumentInfo.java 
> a59d1091b36d0b7908143335cf49d0dafb6627a1 
>   commons/src/main/java/org/apache/aurora/common/args/OptionInfo.java 
> 2fcd3e8f068c4ef950aabe52ca8050dcbd562500 
>   commons/src/main/java/org/apache/aurora/common/args/Parsers.java 
> c4e5fafc49028bd15cfe1bd2a874e73792091cb6 
>   commons/src/main/java/org/apache/aurora/common/args/TypeUtil.java 
> 80cbdd00ce2abb298232793c5fd20c83c6ba6de4 
>   commons/src/main/java/org/apache/aurora/common/args/Verifiers.java 
> 0212873258867ccdb17244e7b0678a7346e79b73 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanExecute.java
>  a26b8a2049a16e96fd97ad2163556de41ba5686e 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanExecuteFileVerifier.java
>  5d9b36070af7c717af5edcc0eec6774c2e26c102 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanRead.java 
> 3fef6a98e4c926126ac93061dd3b32c39f131e3c 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanReadFileVerifier.java
>  8c26304733264b6670e56ab032d7b039f4dfdbfe 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanWrite.java 
> c2beeeb73d475ef9d03e1fde12e6d55983af81ed 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanWriteFileVerifier.java
>  eac273868d72993bc3e83742398c050db2ca4aea 
>   

Re: Review Request 62623: Use a simpler command line argument system

[Stephan Erb]

> On Oct. 4, 2017, 12:29 a.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java
> > Line 106 (original), 110 (patched)
> > 
> >
> > Is Guice doing the injection fully automatically for us here? Am I 
> > missing something?
> 
> Bill Farner wrote:
> Most modules are directly constructed, and the per-module `Options` 
> classes (the name is a convention only, no magic) are manually passed to 
> constructors.  In the case of `ExecutorModule`, it's done in `SchedulerMain`:
> 
> ```java
> Module module = Modules.combine(
> appEnvironmentModule,
> getUniversalModule(options),
> new ServiceDiscoveryModule(
> FlaggedZooKeeperConfig.create(options.zk),
> options.main.serversetPath),
> new BackupModule(options.backup, SnapshotStoreImpl.class),
> new ExecutorModule(options.executor),
> new AbstractModule() {
>   @Override
>   protected void configure() {
> bind(IServerInfo.class).toInstance(
> IServerInfo.build(
> new ServerInfo()
> .setClusterName(options.main.clusterName)
> .setStatsUrlPrefix(options.main.statsUrlPrefix)));
>   }
> });
> ```
> 
> However, some modules are slightly more complex due to the fact that they 
> are dynamically instantiated.  That magic is tucked away in 
> `MoreModules.java`.

Ah, I missed that. Thaks for the explanation.


- Stephan


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


On Sept. 30, 2017, 4:07 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62623/
> ---
> 
> (Updated Sept. 30, 2017, 4:07 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> **NOTE: this patch is not ready to commit, but is ready for initial 
> discussion on the direction**
> 
> This is a very big patch, lots of code removed.  I suggest starting with 
> `CliOptions.java` on page 5, then looking at a few module classes to see the 
> call site changes.
> 
> I made a similar effort a while back, but was unable to finish due to time 
> constraints.  I'm now proposing a more drastic simplification in command line 
> argument handling in the scheduler.  Less magic, more approachable, less 
> brittle with other tools (gradle, IDEs).  The original approach made lots of 
> sense in the original environment where Aurora was developed (a monorepo in a 
> large organization), but this is no longer the case.
> 
> Historical context: 
> https://mail-archives.apache.org/mod_mbox/aurora-dev/201601.mbox/%3CCAFWq12VJPOxDViDP5OsmXU3skHR6rrp5Zp%2BXgi8HgeHic6pTJw%40mail.gmail.com%3E
> 
> 
> Diffs
> -
> 
>   build.gradle f9579a38de95ff9e11cf2e44980d01e103226389 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 4da91591d325ab657dcd37325e5142c65db2ab8c 
>   commons-args/src/main/java/org/apache/aurora/common/args/ArgParser.java 
> 25ed25093bd9defd78df741b6f51e0de0d1f1709 
>   commons-args/src/main/java/org/apache/aurora/common/args/CmdLine.java 
> a72ea7a1669a53d282f9a5a3709add506c9d4b33 
>   commons-args/src/main/java/org/apache/aurora/common/args/NoParser.java 
> 3366531cebe6a79d9c568322f9117d7dbc3e824d 
>   commons-args/src/main/java/org/apache/aurora/common/args/Parser.java 
> 93c23234df38c9f917c0f582b51c25070f996c94 
>   commons-args/src/main/java/org/apache/aurora/common/args/ParserOracle.java 
> e84d3029a45cf7ea1f0bb885788c677ed649b60e 
>   commons-args/src/main/java/org/apache/aurora/common/args/Verifier.java 
> 8f09a9803b8a05b2ab11326d48f26cccaaa278d1 
>   commons-args/src/main/java/org/apache/aurora/common/args/VerifierFor.java 
> aad8807938f569d0dc7a970166ee71ea36f3537d 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/CmdLineProcessor.java
>  5fda5dc6cd8b6d97511073830278850d58bb0bc7 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/Configuration.java
>  1832d41feeafb07f4e7f0bef9dbcb6097e288508 
>   
> commons-args/src/main/resources/META-INF/services/javax.annotation.processing.Processor
>  b548fcdc4389af4420e57908a313435b5300445f 
>   commons/src/main/java/org/apache/aurora/common/args/ArgFilters.java 
> 9fd6eaec98516c3ca9b9a8647af848fcc96509bd 
>   commons/src/main/java/org/apache/aurora/common/args/ArgScanner.java 
> 6e7f23d79deff4983b7feb568320cb938583270b 
>   

Re: Review Request 62623: Use a simpler command line argument system

[David McLaughlin]

> On Sept. 29, 2017, 6:17 p.m., David McLaughlin wrote:
> > I'm also +1 to the spirit of the patch. One requirement to keep in mind 
> > beyond grouping arguments into logical modules is that we also need to make 
> > sure we keep the ability to add custom CLI arguments via the guice 
> > injection mechanism the Scheduler currently provides. (E.g. being able to 
> > add options for pluggable auth, pluggable scheduling, etc.)
> 
> Bill Farner wrote:
> > custom CLI arguments via the guice injection mechanism the Scheduler 
> currently provides
> 
> I see 2 sane approaches for custom options:
> a. maintain a patch that adds these args like others.  this is based on 
> the assumption that a custom aurora module cannot feasibly live outside a 
> fork of the aurora source tree
> b. add a mechanism to specify additional Options classes to statically 
> populate, which may then be consumed by custom modules
> 
> I strongly suggest (a) as considerably simpler in both the aurora 
> codebase and forked codebases.
> 
> David McLaughlin wrote:
> Having to fork individual files causes merge conflicts that have to be 
> manually resolved. For example, we just ran into this with the recent gradle 
> 4.2 changes because we've had to add a couple of extra lines to our 
> build.gradle file, and so that relatively minor commit broke our upstream 
> integration job requiring manual intervention. Not a huge deal, but it's a 
> distraction and hassle if it happens frequently.
> 
> Having to perform a custom build into a local fork of the OSS repo is 
> obviously not a good plugin story in the first place, but for better or worse 
> it is the only real way to customize Aurora at present, and as a heavy user 
> of the custom Guice modules my view would be the benefits introduced here 
> wouldn't be worth the regression that (a) would introduce.
> 
> Bill Farner wrote:
> The patch currently passes `CliOptions` to any dynamically-instantiated 
> module, so how would you feel about an arrangement such as:
> ```java
> public class CustomModule extends AbstractModule {
>   @Parameters(separators = "=")
>   public static class Options {
> @Parameter(names = "-custom_flag")
> public boolean customFlag = false;
>   }
> 
>   static {
> // Statically register custom options for CLI parsing.
> CliOptions.registerCustom(new Options());
>   }
>   
>   public CustomModule(CliOptions options) {
> // Consume parsed and populated options.
> Options customOpts = FluentIterable.from(options.getCustom())
> .firstMatch(Predicates.instanceOf(Options.class));
> System.out.println(customOpts.customFlag);
>   }
> }
> ```

Thanks for the follow-up, this looks fine to me.


- David


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


On Sept. 30, 2017, 2:07 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62623/
> ---
> 
> (Updated Sept. 30, 2017, 2:07 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> **NOTE: this patch is not ready to commit, but is ready for initial 
> discussion on the direction**
> 
> This is a very big patch, lots of code removed.  I suggest starting with 
> `CliOptions.java` on page 5, then looking at a few module classes to see the 
> call site changes.
> 
> I made a similar effort a while back, but was unable to finish due to time 
> constraints.  I'm now proposing a more drastic simplification in command line 
> argument handling in the scheduler.  Less magic, more approachable, less 
> brittle with other tools (gradle, IDEs).  The original approach made lots of 
> sense in the original environment where Aurora was developed (a monorepo in a 
> large organization), but this is no longer the case.
> 
> Historical context: 
> https://mail-archives.apache.org/mod_mbox/aurora-dev/201601.mbox/%3CCAFWq12VJPOxDViDP5OsmXU3skHR6rrp5Zp%2BXgi8HgeHic6pTJw%40mail.gmail.com%3E
> 
> 
> Diffs
> -
> 
>   build.gradle f9579a38de95ff9e11cf2e44980d01e103226389 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 4da91591d325ab657dcd37325e5142c65db2ab8c 
>   commons-args/src/main/java/org/apache/aurora/common/args/ArgParser.java 
> 25ed25093bd9defd78df741b6f51e0de0d1f1709 
>   commons-args/src/main/java/org/apache/aurora/common/args/CmdLine.java 
> a72ea7a1669a53d282f9a5a3709add506c9d4b33 
>   commons-args/src/main/java/org/apache/aurora/common/args/NoParser.java 
> 3366531cebe6a79d9c568322f9117d7dbc3e824d 
>   

Re: Review Request 62623: Use a simpler command line argument system

[Bill Farner]

> On Oct. 3, 2017, 3:29 p.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java
> > Line 106 (original), 110 (patched)
> > 
> >
> > Is Guice doing the injection fully automatically for us here? Am I 
> > missing something?

Most modules are directly constructed, and the per-module `Options` classes 
(the name is a convention only, no magic) are manually passed to constructors.  
In the case of `ExecutorModule`, it's done in `SchedulerMain`:

```java
Module module = Modules.combine(
appEnvironmentModule,
getUniversalModule(options),
new ServiceDiscoveryModule(
FlaggedZooKeeperConfig.create(options.zk),
options.main.serversetPath),
new BackupModule(options.backup, SnapshotStoreImpl.class),
new ExecutorModule(options.executor),
new AbstractModule() {
  @Override
  protected void configure() {
bind(IServerInfo.class).toInstance(
IServerInfo.build(
new ServerInfo()
.setClusterName(options.main.clusterName)
.setStatsUrlPrefix(options.main.statsUrlPrefix)));
  }
});
```

However, some modules are slightly more complex due to the fact that they are 
dynamically instantiated.  That magic is tucked away in `MoreModules.java`.


> On Oct. 3, 2017, 3:29 p.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/Kerberos5ShiroRealmModule.java
> > Lines 71-72 (patched)
> > 
> >
> > Those two are missing the leading hyphen.
> > 
> > In the scheduler `-help` they show up as follows, thus breaking the 
> > existing flags:
> > 
> > ```
> > kerberos_server_keytab
> >   Path to the server keytab.
> > kerberos_server_principal
> >   Kerberos server principal to use, usually of the form
> >   HTTP/aurora.example@example.com
> > ```
> > 
> > Please be so kind to run the end-to-end tests with this patch. Just to 
> > be extra sure we don't have any other problems like these.

> Please be so kind to run the end-to-end tests with this patch

Absolutely!


- Bill


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


On Sept. 29, 2017, 7:07 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62623/
> ---
> 
> (Updated Sept. 29, 2017, 7:07 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> **NOTE: this patch is not ready to commit, but is ready for initial 
> discussion on the direction**
> 
> This is a very big patch, lots of code removed.  I suggest starting with 
> `CliOptions.java` on page 5, then looking at a few module classes to see the 
> call site changes.
> 
> I made a similar effort a while back, but was unable to finish due to time 
> constraints.  I'm now proposing a more drastic simplification in command line 
> argument handling in the scheduler.  Less magic, more approachable, less 
> brittle with other tools (gradle, IDEs).  The original approach made lots of 
> sense in the original environment where Aurora was developed (a monorepo in a 
> large organization), but this is no longer the case.
> 
> Historical context: 
> https://mail-archives.apache.org/mod_mbox/aurora-dev/201601.mbox/%3CCAFWq12VJPOxDViDP5OsmXU3skHR6rrp5Zp%2BXgi8HgeHic6pTJw%40mail.gmail.com%3E
> 
> 
> Diffs
> -
> 
>   build.gradle f9579a38de95ff9e11cf2e44980d01e103226389 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 4da91591d325ab657dcd37325e5142c65db2ab8c 
>   commons-args/src/main/java/org/apache/aurora/common/args/ArgParser.java 
> 25ed25093bd9defd78df741b6f51e0de0d1f1709 
>   commons-args/src/main/java/org/apache/aurora/common/args/CmdLine.java 
> a72ea7a1669a53d282f9a5a3709add506c9d4b33 
>   commons-args/src/main/java/org/apache/aurora/common/args/NoParser.java 
> 3366531cebe6a79d9c568322f9117d7dbc3e824d 
>   commons-args/src/main/java/org/apache/aurora/common/args/Parser.java 
> 93c23234df38c9f917c0f582b51c25070f996c94 
>   commons-args/src/main/java/org/apache/aurora/common/args/ParserOracle.java 
> e84d3029a45cf7ea1f0bb885788c677ed649b60e 
>   commons-args/src/main/java/org/apache/aurora/common/args/Verifier.java 
> 8f09a9803b8a05b2ab11326d48f26cccaaa278d1 
>   commons-args/src/main/java/org/apache/aurora/common/args/VerifierFor.java 
> 

Re: Review Request 62623: Use a simpler command line argument system

[Stephan Erb]

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


Ship it!




The change looks good to me. I did not check everything in detail though.


src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java
Line 106 (original), 110 (patched)


Is Guice doing the injection fully automatically for us here? Am I missing 
something?



src/main/java/org/apache/aurora/scheduler/http/api/security/Kerberos5ShiroRealmModule.java
Lines 71-72 (patched)


Those two are missing the leading hyphen.

In the scheduler `-help` they show up as follows, thus breaking the 
existing flags:

```
kerberos_server_keytab
  Path to the server keytab.
kerberos_server_principal
  Kerberos server principal to use, usually of the form
  HTTP/aurora.example@example.com
```

Please be so kind to run the end-to-end tests with this patch. Just to be 
extra sure we don't have any other problems like these.


- Stephan Erb


On Sept. 30, 2017, 4:07 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62623/
> ---
> 
> (Updated Sept. 30, 2017, 4:07 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> **NOTE: this patch is not ready to commit, but is ready for initial 
> discussion on the direction**
> 
> This is a very big patch, lots of code removed.  I suggest starting with 
> `CliOptions.java` on page 5, then looking at a few module classes to see the 
> call site changes.
> 
> I made a similar effort a while back, but was unable to finish due to time 
> constraints.  I'm now proposing a more drastic simplification in command line 
> argument handling in the scheduler.  Less magic, more approachable, less 
> brittle with other tools (gradle, IDEs).  The original approach made lots of 
> sense in the original environment where Aurora was developed (a monorepo in a 
> large organization), but this is no longer the case.
> 
> Historical context: 
> https://mail-archives.apache.org/mod_mbox/aurora-dev/201601.mbox/%3CCAFWq12VJPOxDViDP5OsmXU3skHR6rrp5Zp%2BXgi8HgeHic6pTJw%40mail.gmail.com%3E
> 
> 
> Diffs
> -
> 
>   build.gradle f9579a38de95ff9e11cf2e44980d01e103226389 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 4da91591d325ab657dcd37325e5142c65db2ab8c 
>   commons-args/src/main/java/org/apache/aurora/common/args/ArgParser.java 
> 25ed25093bd9defd78df741b6f51e0de0d1f1709 
>   commons-args/src/main/java/org/apache/aurora/common/args/CmdLine.java 
> a72ea7a1669a53d282f9a5a3709add506c9d4b33 
>   commons-args/src/main/java/org/apache/aurora/common/args/NoParser.java 
> 3366531cebe6a79d9c568322f9117d7dbc3e824d 
>   commons-args/src/main/java/org/apache/aurora/common/args/Parser.java 
> 93c23234df38c9f917c0f582b51c25070f996c94 
>   commons-args/src/main/java/org/apache/aurora/common/args/ParserOracle.java 
> e84d3029a45cf7ea1f0bb885788c677ed649b60e 
>   commons-args/src/main/java/org/apache/aurora/common/args/Verifier.java 
> 8f09a9803b8a05b2ab11326d48f26cccaaa278d1 
>   commons-args/src/main/java/org/apache/aurora/common/args/VerifierFor.java 
> aad8807938f569d0dc7a970166ee71ea36f3537d 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/CmdLineProcessor.java
>  5fda5dc6cd8b6d97511073830278850d58bb0bc7 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/Configuration.java
>  1832d41feeafb07f4e7f0bef9dbcb6097e288508 
>   
> commons-args/src/main/resources/META-INF/services/javax.annotation.processing.Processor
>  b548fcdc4389af4420e57908a313435b5300445f 
>   commons/src/main/java/org/apache/aurora/common/args/ArgFilters.java 
> 9fd6eaec98516c3ca9b9a8647af848fcc96509bd 
>   commons/src/main/java/org/apache/aurora/common/args/ArgScanner.java 
> 6e7f23d79deff4983b7feb568320cb938583270b 
>   commons/src/main/java/org/apache/aurora/common/args/Args.java 
> 202835debce817df82bd3d860330d23d9710f489 
>   commons/src/main/java/org/apache/aurora/common/args/ArgumentInfo.java 
> a59d1091b36d0b7908143335cf49d0dafb6627a1 
>   commons/src/main/java/org/apache/aurora/common/args/OptionInfo.java 
> 2fcd3e8f068c4ef950aabe52ca8050dcbd562500 
>   commons/src/main/java/org/apache/aurora/common/args/Parsers.java 
> c4e5fafc49028bd15cfe1bd2a874e73792091cb6 
>   commons/src/main/java/org/apache/aurora/common/args/TypeUtil.java 
> 80cbdd00ce2abb298232793c5fd20c83c6ba6de4 
>   commons/src/main/java/org/apache/aurora/common/args/Verifiers.java 
> 

Re: Review Request 62623: Use a simpler command line argument system

[Bill Farner]

> On Sept. 29, 2017, 11:17 a.m., David McLaughlin wrote:
> > I'm also +1 to the spirit of the patch. One requirement to keep in mind 
> > beyond grouping arguments into logical modules is that we also need to make 
> > sure we keep the ability to add custom CLI arguments via the guice 
> > injection mechanism the Scheduler currently provides. (E.g. being able to 
> > add options for pluggable auth, pluggable scheduling, etc.)
> 
> Bill Farner wrote:
> > custom CLI arguments via the guice injection mechanism the Scheduler 
> currently provides
> 
> I see 2 sane approaches for custom options:
> a. maintain a patch that adds these args like others.  this is based on 
> the assumption that a custom aurora module cannot feasibly live outside a 
> fork of the aurora source tree
> b. add a mechanism to specify additional Options classes to statically 
> populate, which may then be consumed by custom modules
> 
> I strongly suggest (a) as considerably simpler in both the aurora 
> codebase and forked codebases.
> 
> David McLaughlin wrote:
> Having to fork individual files causes merge conflicts that have to be 
> manually resolved. For example, we just ran into this with the recent gradle 
> 4.2 changes because we've had to add a couple of extra lines to our 
> build.gradle file, and so that relatively minor commit broke our upstream 
> integration job requiring manual intervention. Not a huge deal, but it's a 
> distraction and hassle if it happens frequently.
> 
> Having to perform a custom build into a local fork of the OSS repo is 
> obviously not a good plugin story in the first place, but for better or worse 
> it is the only real way to customize Aurora at present, and as a heavy user 
> of the custom Guice modules my view would be the benefits introduced here 
> wouldn't be worth the regression that (a) would introduce.

The patch currently passes `CliOptions` to any dynamically-instantiated module, 
so how would you feel about an arrangement such as:
```java
public class CustomModule extends AbstractModule {
  @Parameters(separators = "=")
  public static class Options {
@Parameter(names = "-custom_flag")
public boolean customFlag = false;
  }

  static {
// Statically register custom options for CLI parsing.
CliOptions.registerCustom(new Options());
  }
  
  public CustomModule(CliOptions options) {
// Consume parsed and populated options.
Options customOpts = FluentIterable.from(options.getCustom())
.firstMatch(Predicates.instanceOf(Options.class));
System.out.println(customOpts.customFlag);
  }
}
```


- Bill


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


On Sept. 29, 2017, 7:07 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62623/
> ---
> 
> (Updated Sept. 29, 2017, 7:07 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> **NOTE: this patch is not ready to commit, but is ready for initial 
> discussion on the direction**
> 
> This is a very big patch, lots of code removed.  I suggest starting with 
> `CliOptions.java` on page 5, then looking at a few module classes to see the 
> call site changes.
> 
> I made a similar effort a while back, but was unable to finish due to time 
> constraints.  I'm now proposing a more drastic simplification in command line 
> argument handling in the scheduler.  Less magic, more approachable, less 
> brittle with other tools (gradle, IDEs).  The original approach made lots of 
> sense in the original environment where Aurora was developed (a monorepo in a 
> large organization), but this is no longer the case.
> 
> Historical context: 
> https://mail-archives.apache.org/mod_mbox/aurora-dev/201601.mbox/%3CCAFWq12VJPOxDViDP5OsmXU3skHR6rrp5Zp%2BXgi8HgeHic6pTJw%40mail.gmail.com%3E
> 
> 
> Diffs
> -
> 
>   build.gradle f9579a38de95ff9e11cf2e44980d01e103226389 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 4da91591d325ab657dcd37325e5142c65db2ab8c 
>   commons-args/src/main/java/org/apache/aurora/common/args/ArgParser.java 
> 25ed25093bd9defd78df741b6f51e0de0d1f1709 
>   commons-args/src/main/java/org/apache/aurora/common/args/CmdLine.java 
> a72ea7a1669a53d282f9a5a3709add506c9d4b33 
>   commons-args/src/main/java/org/apache/aurora/common/args/NoParser.java 
> 3366531cebe6a79d9c568322f9117d7dbc3e824d 
>   commons-args/src/main/java/org/apache/aurora/common/args/Parser.java 
> 93c23234df38c9f917c0f582b51c25070f996c94 
>   commons-args/src/main/java/org/apache/aurora/common/args/ParserOracle.java 
> 

Re: Review Request 62623: Use a simpler command line argument system

[David McLaughlin]

> On Sept. 29, 2017, 6:17 p.m., David McLaughlin wrote:
> > I'm also +1 to the spirit of the patch. One requirement to keep in mind 
> > beyond grouping arguments into logical modules is that we also need to make 
> > sure we keep the ability to add custom CLI arguments via the guice 
> > injection mechanism the Scheduler currently provides. (E.g. being able to 
> > add options for pluggable auth, pluggable scheduling, etc.)
> 
> Bill Farner wrote:
> > custom CLI arguments via the guice injection mechanism the Scheduler 
> currently provides
> 
> I see 2 sane approaches for custom options:
> a. maintain a patch that adds these args like others.  this is based on 
> the assumption that a custom aurora module cannot feasibly live outside a 
> fork of the aurora source tree
> b. add a mechanism to specify additional Options classes to statically 
> populate, which may then be consumed by custom modules
> 
> I strongly suggest (a) as considerably simpler in both the aurora 
> codebase and forked codebases.

Having to fork individual files causes merge conflicts that have to be manually 
resolved. For example, we just ran into this with the recent gradle 4.2 changes 
because we've had to add a couple of extra lines to our build.gradle file, and 
so that relatively minor commit broke our upstream integration job requiring 
manual intervention. Not a huge deal, but it's a distraction and hassle if it 
happens frequently.

Having to perform a custom build into a local fork of the OSS repo is obviously 
not a good plugin story in the first place, but for better or worse it is the 
only real way to customize Aurora at present, and as a heavy user of the custom 
Guice modules my view would be the benefits introduced here wouldn't be worth 
the regression that (a) would introduce.


- David


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


On Sept. 30, 2017, 2:07 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62623/
> ---
> 
> (Updated Sept. 30, 2017, 2:07 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> **NOTE: this patch is not ready to commit, but is ready for initial 
> discussion on the direction**
> 
> This is a very big patch, lots of code removed.  I suggest starting with 
> `CliOptions.java` on page 5, then looking at a few module classes to see the 
> call site changes.
> 
> I made a similar effort a while back, but was unable to finish due to time 
> constraints.  I'm now proposing a more drastic simplification in command line 
> argument handling in the scheduler.  Less magic, more approachable, less 
> brittle with other tools (gradle, IDEs).  The original approach made lots of 
> sense in the original environment where Aurora was developed (a monorepo in a 
> large organization), but this is no longer the case.
> 
> Historical context: 
> https://mail-archives.apache.org/mod_mbox/aurora-dev/201601.mbox/%3CCAFWq12VJPOxDViDP5OsmXU3skHR6rrp5Zp%2BXgi8HgeHic6pTJw%40mail.gmail.com%3E
> 
> 
> Diffs
> -
> 
>   build.gradle f9579a38de95ff9e11cf2e44980d01e103226389 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 4da91591d325ab657dcd37325e5142c65db2ab8c 
>   commons-args/src/main/java/org/apache/aurora/common/args/ArgParser.java 
> 25ed25093bd9defd78df741b6f51e0de0d1f1709 
>   commons-args/src/main/java/org/apache/aurora/common/args/CmdLine.java 
> a72ea7a1669a53d282f9a5a3709add506c9d4b33 
>   commons-args/src/main/java/org/apache/aurora/common/args/NoParser.java 
> 3366531cebe6a79d9c568322f9117d7dbc3e824d 
>   commons-args/src/main/java/org/apache/aurora/common/args/Parser.java 
> 93c23234df38c9f917c0f582b51c25070f996c94 
>   commons-args/src/main/java/org/apache/aurora/common/args/ParserOracle.java 
> e84d3029a45cf7ea1f0bb885788c677ed649b60e 
>   commons-args/src/main/java/org/apache/aurora/common/args/Verifier.java 
> 8f09a9803b8a05b2ab11326d48f26cccaaa278d1 
>   commons-args/src/main/java/org/apache/aurora/common/args/VerifierFor.java 
> aad8807938f569d0dc7a970166ee71ea36f3537d 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/CmdLineProcessor.java
>  5fda5dc6cd8b6d97511073830278850d58bb0bc7 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/Configuration.java
>  1832d41feeafb07f4e7f0bef9dbcb6097e288508 
>   
> commons-args/src/main/resources/META-INF/services/javax.annotation.processing.Processor
>  b548fcdc4389af4420e57908a313435b5300445f 
>   commons/src/main/java/org/apache/aurora/common/args/ArgFilters.java 
> 9fd6eaec98516c3ca9b9a8647af848fcc96509bd 
>   

Re: Review Request 62623: Use a simpler command line argument system

[John Sirois]

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


Ship it!




I won't have time to give this a good look until Monday 10/2, but I'm happy 
with a quick skim!

- John Sirois


On Sept. 29, 2017, 8:07 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62623/
> ---
> 
> (Updated Sept. 29, 2017, 8:07 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> **NOTE: this patch is not ready to commit, but is ready for initial 
> discussion on the direction**
> 
> This is a very big patch, lots of code removed.  I suggest starting with 
> `CliOptions.java` on page 5, then looking at a few module classes to see the 
> call site changes.
> 
> I made a similar effort a while back, but was unable to finish due to time 
> constraints.  I'm now proposing a more drastic simplification in command line 
> argument handling in the scheduler.  Less magic, more approachable, less 
> brittle with other tools (gradle, IDEs).  The original approach made lots of 
> sense in the original environment where Aurora was developed (a monorepo in a 
> large organization), but this is no longer the case.
> 
> Historical context: 
> https://mail-archives.apache.org/mod_mbox/aurora-dev/201601.mbox/%3CCAFWq12VJPOxDViDP5OsmXU3skHR6rrp5Zp%2BXgi8HgeHic6pTJw%40mail.gmail.com%3E
> 
> 
> Diffs
> -
> 
>   build.gradle f9579a38de95ff9e11cf2e44980d01e103226389 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 4da91591d325ab657dcd37325e5142c65db2ab8c 
>   commons-args/src/main/java/org/apache/aurora/common/args/ArgParser.java 
> 25ed25093bd9defd78df741b6f51e0de0d1f1709 
>   commons-args/src/main/java/org/apache/aurora/common/args/CmdLine.java 
> a72ea7a1669a53d282f9a5a3709add506c9d4b33 
>   commons-args/src/main/java/org/apache/aurora/common/args/NoParser.java 
> 3366531cebe6a79d9c568322f9117d7dbc3e824d 
>   commons-args/src/main/java/org/apache/aurora/common/args/Parser.java 
> 93c23234df38c9f917c0f582b51c25070f996c94 
>   commons-args/src/main/java/org/apache/aurora/common/args/ParserOracle.java 
> e84d3029a45cf7ea1f0bb885788c677ed649b60e 
>   commons-args/src/main/java/org/apache/aurora/common/args/Verifier.java 
> 8f09a9803b8a05b2ab11326d48f26cccaaa278d1 
>   commons-args/src/main/java/org/apache/aurora/common/args/VerifierFor.java 
> aad8807938f569d0dc7a970166ee71ea36f3537d 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/CmdLineProcessor.java
>  5fda5dc6cd8b6d97511073830278850d58bb0bc7 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/Configuration.java
>  1832d41feeafb07f4e7f0bef9dbcb6097e288508 
>   
> commons-args/src/main/resources/META-INF/services/javax.annotation.processing.Processor
>  b548fcdc4389af4420e57908a313435b5300445f 
>   commons/src/main/java/org/apache/aurora/common/args/ArgFilters.java 
> 9fd6eaec98516c3ca9b9a8647af848fcc96509bd 
>   commons/src/main/java/org/apache/aurora/common/args/ArgScanner.java 
> 6e7f23d79deff4983b7feb568320cb938583270b 
>   commons/src/main/java/org/apache/aurora/common/args/Args.java 
> 202835debce817df82bd3d860330d23d9710f489 
>   commons/src/main/java/org/apache/aurora/common/args/ArgumentInfo.java 
> a59d1091b36d0b7908143335cf49d0dafb6627a1 
>   commons/src/main/java/org/apache/aurora/common/args/OptionInfo.java 
> 2fcd3e8f068c4ef950aabe52ca8050dcbd562500 
>   commons/src/main/java/org/apache/aurora/common/args/Parsers.java 
> c4e5fafc49028bd15cfe1bd2a874e73792091cb6 
>   commons/src/main/java/org/apache/aurora/common/args/TypeUtil.java 
> 80cbdd00ce2abb298232793c5fd20c83c6ba6de4 
>   commons/src/main/java/org/apache/aurora/common/args/Verifiers.java 
> 0212873258867ccdb17244e7b0678a7346e79b73 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanExecute.java
>  a26b8a2049a16e96fd97ad2163556de41ba5686e 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanExecuteFileVerifier.java
>  5d9b36070af7c717af5edcc0eec6774c2e26c102 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanRead.java 
> 3fef6a98e4c926126ac93061dd3b32c39f131e3c 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanReadFileVerifier.java
>  8c26304733264b6670e56ab032d7b039f4dfdbfe 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanWrite.java 
> c2beeeb73d475ef9d03e1fde12e6d55983af81ed 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanWriteFileVerifier.java
>  eac273868d72993bc3e83742398c050db2ca4aea 
>   commons/src/main/java/org/apache/aurora/common/args/constraints/Exists.java 
> 

Re: Review Request 62623: Use a simpler command line argument system

[Aurora ReviewBot]

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


Ship it!




Master (24d2caf) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Sept. 29, 2017, 7:07 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62623/
> ---
> 
> (Updated Sept. 29, 2017, 7:07 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> **NOTE: this patch is not ready to commit, but is ready for initial 
> discussion on the direction**
> 
> This is a very big patch, lots of code removed.  I suggest starting with 
> `CliOptions.java` on page 5, then looking at a few module classes to see the 
> call site changes.
> 
> I made a similar effort a while back, but was unable to finish due to time 
> constraints.  I'm now proposing a more drastic simplification in command line 
> argument handling in the scheduler.  Less magic, more approachable, less 
> brittle with other tools (gradle, IDEs).  The original approach made lots of 
> sense in the original environment where Aurora was developed (a monorepo in a 
> large organization), but this is no longer the case.
> 
> Historical context: 
> https://mail-archives.apache.org/mod_mbox/aurora-dev/201601.mbox/%3CCAFWq12VJPOxDViDP5OsmXU3skHR6rrp5Zp%2BXgi8HgeHic6pTJw%40mail.gmail.com%3E
> 
> 
> Diffs
> -
> 
>   build.gradle f9579a38de95ff9e11cf2e44980d01e103226389 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 4da91591d325ab657dcd37325e5142c65db2ab8c 
>   commons-args/src/main/java/org/apache/aurora/common/args/ArgParser.java 
> 25ed25093bd9defd78df741b6f51e0de0d1f1709 
>   commons-args/src/main/java/org/apache/aurora/common/args/CmdLine.java 
> a72ea7a1669a53d282f9a5a3709add506c9d4b33 
>   commons-args/src/main/java/org/apache/aurora/common/args/NoParser.java 
> 3366531cebe6a79d9c568322f9117d7dbc3e824d 
>   commons-args/src/main/java/org/apache/aurora/common/args/Parser.java 
> 93c23234df38c9f917c0f582b51c25070f996c94 
>   commons-args/src/main/java/org/apache/aurora/common/args/ParserOracle.java 
> e84d3029a45cf7ea1f0bb885788c677ed649b60e 
>   commons-args/src/main/java/org/apache/aurora/common/args/Verifier.java 
> 8f09a9803b8a05b2ab11326d48f26cccaaa278d1 
>   commons-args/src/main/java/org/apache/aurora/common/args/VerifierFor.java 
> aad8807938f569d0dc7a970166ee71ea36f3537d 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/CmdLineProcessor.java
>  5fda5dc6cd8b6d97511073830278850d58bb0bc7 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/Configuration.java
>  1832d41feeafb07f4e7f0bef9dbcb6097e288508 
>   
> commons-args/src/main/resources/META-INF/services/javax.annotation.processing.Processor
>  b548fcdc4389af4420e57908a313435b5300445f 
>   commons/src/main/java/org/apache/aurora/common/args/ArgFilters.java 
> 9fd6eaec98516c3ca9b9a8647af848fcc96509bd 
>   commons/src/main/java/org/apache/aurora/common/args/ArgScanner.java 
> 6e7f23d79deff4983b7feb568320cb938583270b 
>   commons/src/main/java/org/apache/aurora/common/args/Args.java 
> 202835debce817df82bd3d860330d23d9710f489 
>   commons/src/main/java/org/apache/aurora/common/args/ArgumentInfo.java 
> a59d1091b36d0b7908143335cf49d0dafb6627a1 
>   commons/src/main/java/org/apache/aurora/common/args/OptionInfo.java 
> 2fcd3e8f068c4ef950aabe52ca8050dcbd562500 
>   commons/src/main/java/org/apache/aurora/common/args/Parsers.java 
> c4e5fafc49028bd15cfe1bd2a874e73792091cb6 
>   commons/src/main/java/org/apache/aurora/common/args/TypeUtil.java 
> 80cbdd00ce2abb298232793c5fd20c83c6ba6de4 
>   commons/src/main/java/org/apache/aurora/common/args/Verifiers.java 
> 0212873258867ccdb17244e7b0678a7346e79b73 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanExecute.java
>  a26b8a2049a16e96fd97ad2163556de41ba5686e 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanExecuteFileVerifier.java
>  5d9b36070af7c717af5edcc0eec6774c2e26c102 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanRead.java 
> 3fef6a98e4c926126ac93061dd3b32c39f131e3c 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanReadFileVerifier.java
>  8c26304733264b6670e56ab032d7b039f4dfdbfe 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanWrite.java 
> c2beeeb73d475ef9d03e1fde12e6d55983af81ed 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanWriteFileVerifier.java
>  eac273868d72993bc3e83742398c050db2ca4aea 
>   

Re: Review Request 62623: Use a simpler command line argument system

[Aurora ReviewBot]

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



Master (24d2caf) is red with this patch.
  ./build-support/jenkins/build.sh

:findbugsMain
:findbugsTest
:licenseJmh UP-TO-DATE
:licenseMain UP-TO-DATE
:licenseTest UP-TO-DATE
:license UP-TO-DATE
:pmdJmh
:pmdMain
:pmdTest
:test

org.apache.aurora.scheduler.preemptor.PreemptionVictimFilterTest > 
testRevocableVictimsFiltered FAILED
java.lang.IllegalStateException at PreemptionVictimFilterTest.java:138

org.apache.aurora.scheduler.preemptor.PreemptionVictimFilterTest > 
testRevocableVictimRamUsed FAILED
java.lang.IllegalStateException at PreemptionVictimFilterTest.java:138

org.apache.aurora.scheduler.stats.AsyncStatsModuleTest > testOfferAdapter FAILED
java.lang.IllegalStateException at AsyncStatsModuleTest.java:57
I0930 03:16:46.786 [ShutdownHook, SchedulerMain] Stopping scheduler services. 

1137 tests completed, 3 failed, 2 skipped
:test FAILED
:jacocoTestReport
Coverage report generated: 
file:///home/jenkins/jenkins-slave/workspace/AuroraBot/dist/reports/jacoco/test/html/index.html
:jacocoTestCoverageVerification

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':test'.
> There were failing tests. See the report at: 
> file:///home/jenkins/jenkins-slave/workspace/AuroraBot/dist/reports/tests/test/index.html

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug 
option to get more log output.

* Get more help at https://help.gradle.org

BUILD FAILED in 6m 0s
48 actionable tasks: 39 executed, 9 up-to-date


I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Sept. 29, 2017, 7:07 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62623/
> ---
> 
> (Updated Sept. 29, 2017, 7:07 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> **NOTE: this patch is not ready to commit, but is ready for initial 
> discussion on the direction**
> 
> This is a very big patch, lots of code removed.  I suggest starting with 
> `Options.java` on page 1, then looking at a few module classes to see the 
> call site changes.
> 
> I made a similar effort a while back, but was unable to finish due to time 
> constraints.  I'm now proposing a more drastic simplification in command line 
> argument handling in the scheduler.  Less magic, more approachable, less 
> brittle with other tools (gradle, IDEs).  The original approach made lots of 
> sense in the original environment where Aurora was developed (a monorepo in a 
> large organization), but this is no longer the case.
> 
> I'm starting by collecting all options in a single class.  I chose the 
> jcommander library to parse args since it is amenable to having multiple 
> classes that hold args, which could be leveraged to bundle args into separate 
> classes if we choose to do so.
> 
> Historical context: 
> https://mail-archives.apache.org/mod_mbox/aurora-dev/201601.mbox/%3CCAFWq12VJPOxDViDP5OsmXU3skHR6rrp5Zp%2BXgi8HgeHic6pTJw%40mail.gmail.com%3E
> 
> 
> Diffs
> -
> 
>   build.gradle f9579a38de95ff9e11cf2e44980d01e103226389 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 4da91591d325ab657dcd37325e5142c65db2ab8c 
>   commons-args/src/main/java/org/apache/aurora/common/args/ArgParser.java 
> 25ed25093bd9defd78df741b6f51e0de0d1f1709 
>   commons-args/src/main/java/org/apache/aurora/common/args/CmdLine.java 
> a72ea7a1669a53d282f9a5a3709add506c9d4b33 
>   commons-args/src/main/java/org/apache/aurora/common/args/NoParser.java 
> 3366531cebe6a79d9c568322f9117d7dbc3e824d 
>   commons-args/src/main/java/org/apache/aurora/common/args/Parser.java 
> 93c23234df38c9f917c0f582b51c25070f996c94 
>   commons-args/src/main/java/org/apache/aurora/common/args/ParserOracle.java 
> e84d3029a45cf7ea1f0bb885788c677ed649b60e 
>   commons-args/src/main/java/org/apache/aurora/common/args/Verifier.java 
> 8f09a9803b8a05b2ab11326d48f26cccaaa278d1 
>   commons-args/src/main/java/org/apache/aurora/common/args/VerifierFor.java 
> aad8807938f569d0dc7a970166ee71ea36f3537d 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/CmdLineProcessor.java
>  5fda5dc6cd8b6d97511073830278850d58bb0bc7 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/Configuration.java
>  1832d41feeafb07f4e7f0bef9dbcb6097e288508 
>   
> commons-args/src/main/resources/META-INF/services/javax.annotation.processing.Processor
>  b548fcdc4389af4420e57908a313435b5300445f 
>   commons/src/main/java/org/apache/aurora/common/args/ArgFilters.java 

Re: Review Request 62623: Use a simpler command line argument system

[Aurora ReviewBot]

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



Master (24d2caf) is red with this patch.
  ./build-support/jenkins/build.sh

  [86] ./~/react/cjs/react.development.js 55.3 kB {0} [built]
  [87] ./~/react/cjs/react.production.min.js 5.61 kB {0} [built]
+ 75 hidden modules
:processResources
:classes
:jar
:startScripts
:distTar
:distZip
:assemble
:compileTestJava/home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/java/org/apache/aurora/scheduler/thrift/aop/MockDecoratedThrift.java:38:
 Note: Wrote forwarder 
org.apache.aurora.scheduler.thrift.aop.MockDecoratedThriftForwarder
@Forward(AnnotatedAuroraAdmin.class)
^
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.

:processTestResources
:testClasses
:compileJmhJavaNote: 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java
 uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.

:processJmhResources NO-SOURCE
:jmhClasses
:checkstyleJmh
:checkstyleMain[ant:checkstyle] [ERROR] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/app/AppModule.java:80:
 Line is longer than 100 characters (found 101). [LineLength]
 FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':checkstyleMain'.
> Checkstyle rule violations were found. See the report at: 
> file:///home/jenkins/jenkins-slave/workspace/AuroraBot/dist/reports/checkstyle/main.html

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug 
option to get more log output.

* Get more help at https://help.gradle.org

BUILD FAILED in 2m 57s
35 actionable tasks: 29 executed, 6 up-to-date


I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Sept. 30, 2017, 10:07 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62623/
> ---
> 
> (Updated Sept. 30, 2017, 10:07 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> **NOTE: this patch is not ready to commit, but is ready for initial 
> discussion on the direction**
> 
> This is a very big patch, lots of code removed.  I suggest starting with 
> `Options.java` on page 1, then looking at a few module classes to see the 
> call site changes.
> 
> I made a similar effort a while back, but was unable to finish due to time 
> constraints.  I'm now proposing a more drastic simplification in command line 
> argument handling in the scheduler.  Less magic, more approachable, less 
> brittle with other tools (gradle, IDEs).  The original approach made lots of 
> sense in the original environment where Aurora was developed (a monorepo in a 
> large organization), but this is no longer the case.
> 
> I'm starting by collecting all options in a single class.  I chose the 
> jcommander library to parse args since it is amenable to having multiple 
> classes that hold args, which could be leveraged to bundle args into separate 
> classes if we choose to do so.
> 
> Historical context: 
> https://mail-archives.apache.org/mod_mbox/aurora-dev/201601.mbox/%3CCAFWq12VJPOxDViDP5OsmXU3skHR6rrp5Zp%2BXgi8HgeHic6pTJw%40mail.gmail.com%3E
> 
> 
> Diffs
> -
> 
>   build.gradle f9579a38de95ff9e11cf2e44980d01e103226389 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 4da91591d325ab657dcd37325e5142c65db2ab8c 
>   commons-args/src/main/java/org/apache/aurora/common/args/ArgParser.java 
> 25ed25093bd9defd78df741b6f51e0de0d1f1709 
>   commons-args/src/main/java/org/apache/aurora/common/args/CmdLine.java 
> a72ea7a1669a53d282f9a5a3709add506c9d4b33 
>   commons-args/src/main/java/org/apache/aurora/common/args/NoParser.java 
> 3366531cebe6a79d9c568322f9117d7dbc3e824d 
>   commons-args/src/main/java/org/apache/aurora/common/args/Parser.java 
> 93c23234df38c9f917c0f582b51c25070f996c94 
>   commons-args/src/main/java/org/apache/aurora/common/args/ParserOracle.java 
> e84d3029a45cf7ea1f0bb885788c677ed649b60e 
>   commons-args/src/main/java/org/apache/aurora/common/args/Verifier.java 
> 8f09a9803b8a05b2ab11326d48f26cccaaa278d1 
>   commons-args/src/main/java/org/apache/aurora/common/args/VerifierFor.java 
> aad8807938f569d0dc7a970166ee71ea36f3537d 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/CmdLineProcessor.java
>  5fda5dc6cd8b6d97511073830278850d58bb0bc7 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/Configuration.java
>  

Re: Review Request 62623: Use a simpler command line argument system

[Bill Farner]

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

(Updated Sept. 29, 2017, 7:07 p.m.)


Review request for Aurora, David McLaughlin and John Sirois.


Changes
---

This draft moves args back to Module classes, and eliminates all but one of the 
static options accesses.

Still not ready for commit, but this is closer to a shippable shape.


Repository: aurora


Description
---

**NOTE: this patch is not ready to commit, but is ready for initial discussion 
on the direction**

This is a very big patch, lots of code removed.  I suggest starting with 
`Options.java` on page 1, then looking at a few module classes to see the call 
site changes.

I made a similar effort a while back, but was unable to finish due to time 
constraints.  I'm now proposing a more drastic simplification in command line 
argument handling in the scheduler.  Less magic, more approachable, less 
brittle with other tools (gradle, IDEs).  The original approach made lots of 
sense in the original environment where Aurora was developed (a monorepo in a 
large organization), but this is no longer the case.

I'm starting by collecting all options in a single class.  I chose the 
jcommander library to parse args since it is amenable to having multiple 
classes that hold args, which could be leveraged to bundle args into separate 
classes if we choose to do so.

Historical context: 
https://mail-archives.apache.org/mod_mbox/aurora-dev/201601.mbox/%3CCAFWq12VJPOxDViDP5OsmXU3skHR6rrp5Zp%2BXgi8HgeHic6pTJw%40mail.gmail.com%3E


Diffs (updated)
-

  build.gradle f9579a38de95ff9e11cf2e44980d01e103226389 
  commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
4da91591d325ab657dcd37325e5142c65db2ab8c 
  commons-args/src/main/java/org/apache/aurora/common/args/ArgParser.java 
25ed25093bd9defd78df741b6f51e0de0d1f1709 
  commons-args/src/main/java/org/apache/aurora/common/args/CmdLine.java 
a72ea7a1669a53d282f9a5a3709add506c9d4b33 
  commons-args/src/main/java/org/apache/aurora/common/args/NoParser.java 
3366531cebe6a79d9c568322f9117d7dbc3e824d 
  commons-args/src/main/java/org/apache/aurora/common/args/Parser.java 
93c23234df38c9f917c0f582b51c25070f996c94 
  commons-args/src/main/java/org/apache/aurora/common/args/ParserOracle.java 
e84d3029a45cf7ea1f0bb885788c677ed649b60e 
  commons-args/src/main/java/org/apache/aurora/common/args/Verifier.java 
8f09a9803b8a05b2ab11326d48f26cccaaa278d1 
  commons-args/src/main/java/org/apache/aurora/common/args/VerifierFor.java 
aad8807938f569d0dc7a970166ee71ea36f3537d 
  
commons-args/src/main/java/org/apache/aurora/common/args/apt/CmdLineProcessor.java
 5fda5dc6cd8b6d97511073830278850d58bb0bc7 
  
commons-args/src/main/java/org/apache/aurora/common/args/apt/Configuration.java 
1832d41feeafb07f4e7f0bef9dbcb6097e288508 
  
commons-args/src/main/resources/META-INF/services/javax.annotation.processing.Processor
 b548fcdc4389af4420e57908a313435b5300445f 
  commons/src/main/java/org/apache/aurora/common/args/ArgFilters.java 
9fd6eaec98516c3ca9b9a8647af848fcc96509bd 
  commons/src/main/java/org/apache/aurora/common/args/ArgScanner.java 
6e7f23d79deff4983b7feb568320cb938583270b 
  commons/src/main/java/org/apache/aurora/common/args/Args.java 
202835debce817df82bd3d860330d23d9710f489 
  commons/src/main/java/org/apache/aurora/common/args/ArgumentInfo.java 
a59d1091b36d0b7908143335cf49d0dafb6627a1 
  commons/src/main/java/org/apache/aurora/common/args/OptionInfo.java 
2fcd3e8f068c4ef950aabe52ca8050dcbd562500 
  commons/src/main/java/org/apache/aurora/common/args/Parsers.java 
c4e5fafc49028bd15cfe1bd2a874e73792091cb6 
  commons/src/main/java/org/apache/aurora/common/args/TypeUtil.java 
80cbdd00ce2abb298232793c5fd20c83c6ba6de4 
  commons/src/main/java/org/apache/aurora/common/args/Verifiers.java 
0212873258867ccdb17244e7b0678a7346e79b73 
  
commons/src/main/java/org/apache/aurora/common/args/constraints/CanExecute.java 
a26b8a2049a16e96fd97ad2163556de41ba5686e 
  
commons/src/main/java/org/apache/aurora/common/args/constraints/CanExecuteFileVerifier.java
 5d9b36070af7c717af5edcc0eec6774c2e26c102 
  commons/src/main/java/org/apache/aurora/common/args/constraints/CanRead.java 
3fef6a98e4c926126ac93061dd3b32c39f131e3c 
  
commons/src/main/java/org/apache/aurora/common/args/constraints/CanReadFileVerifier.java
 8c26304733264b6670e56ab032d7b039f4dfdbfe 
  commons/src/main/java/org/apache/aurora/common/args/constraints/CanWrite.java 
c2beeeb73d475ef9d03e1fde12e6d55983af81ed 
  
commons/src/main/java/org/apache/aurora/common/args/constraints/CanWriteFileVerifier.java
 eac273868d72993bc3e83742398c050db2ca4aea 
  commons/src/main/java/org/apache/aurora/common/args/constraints/Exists.java 
217d10e561b175885d6bb2a058225935fd88b37a 
  
commons/src/main/java/org/apache/aurora/common/args/constraints/ExistsFileVerifier.java
 

Re: Review Request 62623: Use a simpler command line argument system

[Bill Farner]

> On Sept. 29, 2017, 11:17 a.m., David McLaughlin wrote:
> > I'm also +1 to the spirit of the patch. One requirement to keep in mind 
> > beyond grouping arguments into logical modules is that we also need to make 
> > sure we keep the ability to add custom CLI arguments via the guice 
> > injection mechanism the Scheduler currently provides. (E.g. being able to 
> > add options for pluggable auth, pluggable scheduling, etc.)

> custom CLI arguments via the guice injection mechanism the Scheduler 
> currently provides

I see 2 sane approaches for custom options:
a. maintain a patch that adds these args like others.  this is based on the 
assumption that a custom aurora module cannot feasibly live outside a fork of 
the aurora source tree
b. add a mechanism to specify additional Options classes to statically 
populate, which may then be consumed by custom modules

I strongly suggest (a) as considerably simpler in both the aurora codebase and 
forked codebases.


- Bill


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


On Sept. 27, 2017, 9:22 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62623/
> ---
> 
> (Updated Sept. 27, 2017, 9:22 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> **NOTE: this patch is not ready to commit, but is ready for initial 
> discussion on the direction**
> 
> This is a very big patch, lots of code removed.  I suggest starting with 
> `Options.java` on page 1, then looking at a few module classes to see the 
> call site changes.
> 
> I made a similar effort a while back, but was unable to finish due to time 
> constraints.  I'm now proposing a more drastic simplification in command line 
> argument handling in the scheduler.  Less magic, more approachable, less 
> brittle with other tools (gradle, IDEs).  The original approach made lots of 
> sense in the original environment where Aurora was developed (a monorepo in a 
> large organization), but this is no longer the case.
> 
> I'm starting by collecting all options in a single class.  I chose the 
> jcommander library to parse args since it is amenable to having multiple 
> classes that hold args, which could be leveraged to bundle args into separate 
> classes if we choose to do so.
> 
> Historical context: 
> https://mail-archives.apache.org/mod_mbox/aurora-dev/201601.mbox/%3CCAFWq12VJPOxDViDP5OsmXU3skHR6rrp5Zp%2BXgi8HgeHic6pTJw%40mail.gmail.com%3E
> 
> 
> Diffs
> -
> 
>   build.gradle 460500aaeab28dcfeb29ec602d057ea4bca12378 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 4da91591d325ab657dcd37325e5142c65db2ab8c 
>   commons-args/src/main/java/org/apache/aurora/common/args/ArgParser.java 
> 25ed25093bd9defd78df741b6f51e0de0d1f1709 
>   commons-args/src/main/java/org/apache/aurora/common/args/CmdLine.java 
> a72ea7a1669a53d282f9a5a3709add506c9d4b33 
>   commons-args/src/main/java/org/apache/aurora/common/args/NoParser.java 
> 3366531cebe6a79d9c568322f9117d7dbc3e824d 
>   commons-args/src/main/java/org/apache/aurora/common/args/Parser.java 
> 93c23234df38c9f917c0f582b51c25070f996c94 
>   commons-args/src/main/java/org/apache/aurora/common/args/ParserOracle.java 
> e84d3029a45cf7ea1f0bb885788c677ed649b60e 
>   commons-args/src/main/java/org/apache/aurora/common/args/Verifier.java 
> 8f09a9803b8a05b2ab11326d48f26cccaaa278d1 
>   commons-args/src/main/java/org/apache/aurora/common/args/VerifierFor.java 
> aad8807938f569d0dc7a970166ee71ea36f3537d 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/CmdLineProcessor.java
>  5fda5dc6cd8b6d97511073830278850d58bb0bc7 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/Configuration.java
>  1832d41feeafb07f4e7f0bef9dbcb6097e288508 
>   
> commons-args/src/main/resources/META-INF/services/javax.annotation.processing.Processor
>  b548fcdc4389af4420e57908a313435b5300445f 
>   commons/src/main/java/org/apache/aurora/common/args/ArgFilters.java 
> 9fd6eaec98516c3ca9b9a8647af848fcc96509bd 
>   commons/src/main/java/org/apache/aurora/common/args/ArgScanner.java 
> 6e7f23d79deff4983b7feb568320cb938583270b 
>   commons/src/main/java/org/apache/aurora/common/args/Args.java 
> 202835debce817df82bd3d860330d23d9710f489 
>   commons/src/main/java/org/apache/aurora/common/args/ArgumentInfo.java 
> a59d1091b36d0b7908143335cf49d0dafb6627a1 
>   commons/src/main/java/org/apache/aurora/common/args/OptionInfo.java 
> 2fcd3e8f068c4ef950aabe52ca8050dcbd562500 
>   commons/src/main/java/org/apache/aurora/common/args/Parsers.java 
> c4e5fafc49028bd15cfe1bd2a874e73792091cb6 
>   

Re: Review Request 62623: Use a simpler command line argument system

[David McLaughlin]

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



I'm also +1 to the spirit of the patch. One requirement to keep in mind beyond 
grouping arguments into logical modules is that we also need to make sure we 
keep the ability to add custom CLI arguments via the guice injection mechanism 
the Scheduler currently provides. (E.g. being able to add options for pluggable 
auth, pluggable scheduling, etc.)

- David McLaughlin


On Sept. 28, 2017, 4:22 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62623/
> ---
> 
> (Updated Sept. 28, 2017, 4:22 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> **NOTE: this patch is not ready to commit, but is ready for initial 
> discussion on the direction**
> 
> This is a very big patch, lots of code removed.  I suggest starting with 
> `Options.java` on page 1, then looking at a few module classes to see the 
> call site changes.
> 
> I made a similar effort a while back, but was unable to finish due to time 
> constraints.  I'm now proposing a more drastic simplification in command line 
> argument handling in the scheduler.  Less magic, more approachable, less 
> brittle with other tools (gradle, IDEs).  The original approach made lots of 
> sense in the original environment where Aurora was developed (a monorepo in a 
> large organization), but this is no longer the case.
> 
> I'm starting by collecting all options in a single class.  I chose the 
> jcommander library to parse args since it is amenable to having multiple 
> classes that hold args, which could be leveraged to bundle args into separate 
> classes if we choose to do so.
> 
> Historical context: 
> https://mail-archives.apache.org/mod_mbox/aurora-dev/201601.mbox/%3CCAFWq12VJPOxDViDP5OsmXU3skHR6rrp5Zp%2BXgi8HgeHic6pTJw%40mail.gmail.com%3E
> 
> 
> Diffs
> -
> 
>   build.gradle 460500aaeab28dcfeb29ec602d057ea4bca12378 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 4da91591d325ab657dcd37325e5142c65db2ab8c 
>   commons-args/src/main/java/org/apache/aurora/common/args/ArgParser.java 
> 25ed25093bd9defd78df741b6f51e0de0d1f1709 
>   commons-args/src/main/java/org/apache/aurora/common/args/CmdLine.java 
> a72ea7a1669a53d282f9a5a3709add506c9d4b33 
>   commons-args/src/main/java/org/apache/aurora/common/args/NoParser.java 
> 3366531cebe6a79d9c568322f9117d7dbc3e824d 
>   commons-args/src/main/java/org/apache/aurora/common/args/Parser.java 
> 93c23234df38c9f917c0f582b51c25070f996c94 
>   commons-args/src/main/java/org/apache/aurora/common/args/ParserOracle.java 
> e84d3029a45cf7ea1f0bb885788c677ed649b60e 
>   commons-args/src/main/java/org/apache/aurora/common/args/Verifier.java 
> 8f09a9803b8a05b2ab11326d48f26cccaaa278d1 
>   commons-args/src/main/java/org/apache/aurora/common/args/VerifierFor.java 
> aad8807938f569d0dc7a970166ee71ea36f3537d 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/CmdLineProcessor.java
>  5fda5dc6cd8b6d97511073830278850d58bb0bc7 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/Configuration.java
>  1832d41feeafb07f4e7f0bef9dbcb6097e288508 
>   
> commons-args/src/main/resources/META-INF/services/javax.annotation.processing.Processor
>  b548fcdc4389af4420e57908a313435b5300445f 
>   commons/src/main/java/org/apache/aurora/common/args/ArgFilters.java 
> 9fd6eaec98516c3ca9b9a8647af848fcc96509bd 
>   commons/src/main/java/org/apache/aurora/common/args/ArgScanner.java 
> 6e7f23d79deff4983b7feb568320cb938583270b 
>   commons/src/main/java/org/apache/aurora/common/args/Args.java 
> 202835debce817df82bd3d860330d23d9710f489 
>   commons/src/main/java/org/apache/aurora/common/args/ArgumentInfo.java 
> a59d1091b36d0b7908143335cf49d0dafb6627a1 
>   commons/src/main/java/org/apache/aurora/common/args/OptionInfo.java 
> 2fcd3e8f068c4ef950aabe52ca8050dcbd562500 
>   commons/src/main/java/org/apache/aurora/common/args/Parsers.java 
> c4e5fafc49028bd15cfe1bd2a874e73792091cb6 
>   commons/src/main/java/org/apache/aurora/common/args/TypeUtil.java 
> 80cbdd00ce2abb298232793c5fd20c83c6ba6de4 
>   commons/src/main/java/org/apache/aurora/common/args/Verifiers.java 
> 0212873258867ccdb17244e7b0678a7346e79b73 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanExecute.java
>  a26b8a2049a16e96fd97ad2163556de41ba5686e 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanExecuteFileVerifier.java
>  5d9b36070af7c717af5edcc0eec6774c2e26c102 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanRead.java 
> 

Re: Review Request 62623: Use a simpler command line argument system

[Bill Farner]

> On Sept. 29, 2017, 10:03 a.m., Jordan Ly wrote:
> > I am definitely +1 to the spirit of this change.
> > 
> > I would prefer to go the route of multiple classes that hold arguments in 
> > logical groupings (ex. ReplicatedLogArgs and etc. along those lines). Do 
> > any modules in the scheduler share arguments? How is that done today? I am 
> > not too familiar with JCommander/argument parsing libraries but I was 
> > wondering how much functionality it gives out of the box for us right now 
> > vs. how many custom parsers we will have to write.
> 
> Jordan Ly wrote:
> The questions are mostly for my curiosity, and splitting the options 
> up/adding parsers can definitely come in later changes.
> 
> Bill Farner wrote:
> > I would prefer to go the route of multiple classes that hold arguments 
> in logical groupings
> 
> I was really torn on this.  Organization is certainly lost in the global 
> `Options` class.  I may group them once everything is working in this patch, 
> but i didn't want to distract with housekeeping.
> 
> > Do any modules in the scheduler share arguments?
> 
> Yes, but only in a few outlier cases.
> 
> > How is that done today?
> 
> Two ways - collecting the arg values into a settings class and passing 
> that around, or the cowboy way of exposing the static arg field and reading 
> it from another class.
> 
> > how many custom parsers we will have to write
> 
> I count ~13 parsers total, ~all of which i can port from what's currently 
> on master.  This doesn't phase me.  In my mind, the more difficult decision 
> is how to collect and inject all the arg values into the code that uses them.

I think i have a sane approach for grouping options and putting them back in 
the modules, so i'll update the patch to do so.  I will create a dependency 
cycle between classes that i don't love, but retains the grouping and will make 
the code review more sane!


- Bill


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


On Sept. 27, 2017, 9:22 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62623/
> ---
> 
> (Updated Sept. 27, 2017, 9:22 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> **NOTE: this patch is not ready to commit, but is ready for initial 
> discussion on the direction**
> 
> This is a very big patch, lots of code removed.  I suggest starting with 
> `Options.java` on page 1, then looking at a few module classes to see the 
> call site changes.
> 
> I made a similar effort a while back, but was unable to finish due to time 
> constraints.  I'm now proposing a more drastic simplification in command line 
> argument handling in the scheduler.  Less magic, more approachable, less 
> brittle with other tools (gradle, IDEs).  The original approach made lots of 
> sense in the original environment where Aurora was developed (a monorepo in a 
> large organization), but this is no longer the case.
> 
> I'm starting by collecting all options in a single class.  I chose the 
> jcommander library to parse args since it is amenable to having multiple 
> classes that hold args, which could be leveraged to bundle args into separate 
> classes if we choose to do so.
> 
> Historical context: 
> https://mail-archives.apache.org/mod_mbox/aurora-dev/201601.mbox/%3CCAFWq12VJPOxDViDP5OsmXU3skHR6rrp5Zp%2BXgi8HgeHic6pTJw%40mail.gmail.com%3E
> 
> 
> Diffs
> -
> 
>   build.gradle 460500aaeab28dcfeb29ec602d057ea4bca12378 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 4da91591d325ab657dcd37325e5142c65db2ab8c 
>   commons-args/src/main/java/org/apache/aurora/common/args/ArgParser.java 
> 25ed25093bd9defd78df741b6f51e0de0d1f1709 
>   commons-args/src/main/java/org/apache/aurora/common/args/CmdLine.java 
> a72ea7a1669a53d282f9a5a3709add506c9d4b33 
>   commons-args/src/main/java/org/apache/aurora/common/args/NoParser.java 
> 3366531cebe6a79d9c568322f9117d7dbc3e824d 
>   commons-args/src/main/java/org/apache/aurora/common/args/Parser.java 
> 93c23234df38c9f917c0f582b51c25070f996c94 
>   commons-args/src/main/java/org/apache/aurora/common/args/ParserOracle.java 
> e84d3029a45cf7ea1f0bb885788c677ed649b60e 
>   commons-args/src/main/java/org/apache/aurora/common/args/Verifier.java 
> 8f09a9803b8a05b2ab11326d48f26cccaaa278d1 
>   commons-args/src/main/java/org/apache/aurora/common/args/VerifierFor.java 
> aad8807938f569d0dc7a970166ee71ea36f3537d 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/CmdLineProcessor.java
>  

Re: Review Request 62623: Use a simpler command line argument system

[Bill Farner]

> On Sept. 28, 2017, 10:15 p.m., Santhosh Kumar Shanmugham wrote:
> > src/main/java/org/apache/aurora/scheduler/log/mesos/MesosLogStreamModule.java
> > Line 55 (original), 54 (patched)
> > 
> >
> > Can we delegate this constraint checking to `JCommander`?
> 
> Bill Farner wrote:
> I've found this tends to be awkward (in all arg libs i've seen) for args 
> that are required based on the value of other args, which is the case here.
> 
> Santhosh Kumar Shanmugham wrote:
> Sounds like we need a library with subcommand support.
> 
> Bill Farner wrote:
> JCommander does support subcommands, but i don't see how that helps here. 
>  A subcommand is a different application entrypoint (do thing X _or_ do thing 
> Y).  Here we have `--enable-feature-y` and `--required-flag-for-feature-y`.
> 
> Santhosh Kumar Shanmugham wrote:
> Sorry. I meant something like `requiredIf` validation, like `JOpt` 
> provides :)

Aha, now *that* would be nice!


- Bill


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


On Sept. 27, 2017, 9:22 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62623/
> ---
> 
> (Updated Sept. 27, 2017, 9:22 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> **NOTE: this patch is not ready to commit, but is ready for initial 
> discussion on the direction**
> 
> This is a very big patch, lots of code removed.  I suggest starting with 
> `Options.java` on page 1, then looking at a few module classes to see the 
> call site changes.
> 
> I made a similar effort a while back, but was unable to finish due to time 
> constraints.  I'm now proposing a more drastic simplification in command line 
> argument handling in the scheduler.  Less magic, more approachable, less 
> brittle with other tools (gradle, IDEs).  The original approach made lots of 
> sense in the original environment where Aurora was developed (a monorepo in a 
> large organization), but this is no longer the case.
> 
> I'm starting by collecting all options in a single class.  I chose the 
> jcommander library to parse args since it is amenable to having multiple 
> classes that hold args, which could be leveraged to bundle args into separate 
> classes if we choose to do so.
> 
> Historical context: 
> https://mail-archives.apache.org/mod_mbox/aurora-dev/201601.mbox/%3CCAFWq12VJPOxDViDP5OsmXU3skHR6rrp5Zp%2BXgi8HgeHic6pTJw%40mail.gmail.com%3E
> 
> 
> Diffs
> -
> 
>   build.gradle 460500aaeab28dcfeb29ec602d057ea4bca12378 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 4da91591d325ab657dcd37325e5142c65db2ab8c 
>   commons-args/src/main/java/org/apache/aurora/common/args/ArgParser.java 
> 25ed25093bd9defd78df741b6f51e0de0d1f1709 
>   commons-args/src/main/java/org/apache/aurora/common/args/CmdLine.java 
> a72ea7a1669a53d282f9a5a3709add506c9d4b33 
>   commons-args/src/main/java/org/apache/aurora/common/args/NoParser.java 
> 3366531cebe6a79d9c568322f9117d7dbc3e824d 
>   commons-args/src/main/java/org/apache/aurora/common/args/Parser.java 
> 93c23234df38c9f917c0f582b51c25070f996c94 
>   commons-args/src/main/java/org/apache/aurora/common/args/ParserOracle.java 
> e84d3029a45cf7ea1f0bb885788c677ed649b60e 
>   commons-args/src/main/java/org/apache/aurora/common/args/Verifier.java 
> 8f09a9803b8a05b2ab11326d48f26cccaaa278d1 
>   commons-args/src/main/java/org/apache/aurora/common/args/VerifierFor.java 
> aad8807938f569d0dc7a970166ee71ea36f3537d 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/CmdLineProcessor.java
>  5fda5dc6cd8b6d97511073830278850d58bb0bc7 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/Configuration.java
>  1832d41feeafb07f4e7f0bef9dbcb6097e288508 
>   
> commons-args/src/main/resources/META-INF/services/javax.annotation.processing.Processor
>  b548fcdc4389af4420e57908a313435b5300445f 
>   commons/src/main/java/org/apache/aurora/common/args/ArgFilters.java 
> 9fd6eaec98516c3ca9b9a8647af848fcc96509bd 
>   commons/src/main/java/org/apache/aurora/common/args/ArgScanner.java 
> 6e7f23d79deff4983b7feb568320cb938583270b 
>   commons/src/main/java/org/apache/aurora/common/args/Args.java 
> 202835debce817df82bd3d860330d23d9710f489 
>   commons/src/main/java/org/apache/aurora/common/args/ArgumentInfo.java 
> a59d1091b36d0b7908143335cf49d0dafb6627a1 
>   commons/src/main/java/org/apache/aurora/common/args/OptionInfo.java 
> 2fcd3e8f068c4ef950aabe52ca8050dcbd562500 
>   

Re: Review Request 62623: Use a simpler command line argument system

[Santhosh Kumar Shanmugham]

> On Sept. 28, 2017, 10:15 p.m., Santhosh Kumar Shanmugham wrote:
> > Mostly nits around parameter validation.
> 
> Bill Farner wrote:
> My apologies, i could have been more clear on the _not ready to commit_ 
> line.  The commit needs a lot of polish, including parsers.  At this point 
> i'm looking for feedback for the shape of things.  Thanks for the thorough 
> look either way!
> 
> Santhosh Kumar Shanmugham wrote:
> Ah, my bad I missed the note.
> 
> +1 for removing commandline parsing code and consolidating the options in 
> a single place. Makes it much easier to traverse code.
> 
> However, what was the motivation for picking `JCommander`. Have you 
> considered `JOpt` or plain old `Jakarta-cli`? I ask since I find `JCommander` 
> to be limited.
> 
> Bill Farner wrote:
> > what was the motivation for picking JCommander
> 
> I like that JCommander requires explicit and up-front specificiation of 
> args, types, and parsers.  This in contrast to getopt-style parsers that 
> require calling code to fetch args by a name string, and deal with types and 
> parsing at the call site.  Since we have over 100 args, we would invariably 
> end up wanting to abstract away and centralize such behavior, ultimately 
> finding ourselves writing another args lib.
> 
> > Have you considered JOpt or plain old Jakarta-cli
> 
> I discarded [JOpt Simple](https://pholser.github.io/jopt-simple/) and 
> [commons-cli](https://commons.apache.org/proper/commons-cli/) (i assume 
> that's what you mean by Jakarta-cli?) because they are both getopt-style 
> libraries.

That seems reasonable.


> On Sept. 28, 2017, 10:15 p.m., Santhosh Kumar Shanmugham wrote:
> > src/main/java/org/apache/aurora/scheduler/log/mesos/MesosLogStreamModule.java
> > Line 55 (original), 54 (patched)
> > 
> >
> > Can we delegate this constraint checking to `JCommander`?
> 
> Bill Farner wrote:
> I've found this tends to be awkward (in all arg libs i've seen) for args 
> that are required based on the value of other args, which is the case here.
> 
> Santhosh Kumar Shanmugham wrote:
> Sounds like we need a library with subcommand support.
> 
> Bill Farner wrote:
> JCommander does support subcommands, but i don't see how that helps here. 
>  A subcommand is a different application entrypoint (do thing X _or_ do thing 
> Y).  Here we have `--enable-feature-y` and `--required-flag-for-feature-y`.

Sorry. I meant something like `requiredIf` validation, like `JOpt` provides :)


- Santhosh Kumar


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


On Sept. 27, 2017, 9:22 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62623/
> ---
> 
> (Updated Sept. 27, 2017, 9:22 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> **NOTE: this patch is not ready to commit, but is ready for initial 
> discussion on the direction**
> 
> This is a very big patch, lots of code removed.  I suggest starting with 
> `Options.java` on page 1, then looking at a few module classes to see the 
> call site changes.
> 
> I made a similar effort a while back, but was unable to finish due to time 
> constraints.  I'm now proposing a more drastic simplification in command line 
> argument handling in the scheduler.  Less magic, more approachable, less 
> brittle with other tools (gradle, IDEs).  The original approach made lots of 
> sense in the original environment where Aurora was developed (a monorepo in a 
> large organization), but this is no longer the case.
> 
> I'm starting by collecting all options in a single class.  I chose the 
> jcommander library to parse args since it is amenable to having multiple 
> classes that hold args, which could be leveraged to bundle args into separate 
> classes if we choose to do so.
> 
> Historical context: 
> https://mail-archives.apache.org/mod_mbox/aurora-dev/201601.mbox/%3CCAFWq12VJPOxDViDP5OsmXU3skHR6rrp5Zp%2BXgi8HgeHic6pTJw%40mail.gmail.com%3E
> 
> 
> Diffs
> -
> 
>   build.gradle 460500aaeab28dcfeb29ec602d057ea4bca12378 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 4da91591d325ab657dcd37325e5142c65db2ab8c 
>   commons-args/src/main/java/org/apache/aurora/common/args/ArgParser.java 
> 25ed25093bd9defd78df741b6f51e0de0d1f1709 
>   commons-args/src/main/java/org/apache/aurora/common/args/CmdLine.java 
> a72ea7a1669a53d282f9a5a3709add506c9d4b33 
>   commons-args/src/main/java/org/apache/aurora/common/args/NoParser.java 
> 

Re: Review Request 62623: Use a simpler command line argument system

[Bill Farner]

> On Sept. 29, 2017, 10:03 a.m., Jordan Ly wrote:
> > I am definitely +1 to the spirit of this change.
> > 
> > I would prefer to go the route of multiple classes that hold arguments in 
> > logical groupings (ex. ReplicatedLogArgs and etc. along those lines). Do 
> > any modules in the scheduler share arguments? How is that done today? I am 
> > not too familiar with JCommander/argument parsing libraries but I was 
> > wondering how much functionality it gives out of the box for us right now 
> > vs. how many custom parsers we will have to write.
> 
> Jordan Ly wrote:
> The questions are mostly for my curiosity, and splitting the options 
> up/adding parsers can definitely come in later changes.

> I would prefer to go the route of multiple classes that hold arguments in 
> logical groupings

I was really torn on this.  Organization is certainly lost in the global 
`Options` class.  I may group them once everything is working in this patch, 
but i didn't want to distract with housekeeping.

> Do any modules in the scheduler share arguments?

Yes, but only in a few outlier cases.

> How is that done today?

Two ways - collecting the arg values into a settings class and passing that 
around, or the cowboy way of exposing the static arg field and reading it from 
another class.

> how many custom parsers we will have to write

I count ~13 parsers total, ~all of which i can port from what's currently on 
master.  This doesn't phase me.  In my mind, the more difficult decision is how 
to collect and inject all the arg values into the code that uses them.


- Bill


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


On Sept. 27, 2017, 9:22 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62623/
> ---
> 
> (Updated Sept. 27, 2017, 9:22 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> **NOTE: this patch is not ready to commit, but is ready for initial 
> discussion on the direction**
> 
> This is a very big patch, lots of code removed.  I suggest starting with 
> `Options.java` on page 1, then looking at a few module classes to see the 
> call site changes.
> 
> I made a similar effort a while back, but was unable to finish due to time 
> constraints.  I'm now proposing a more drastic simplification in command line 
> argument handling in the scheduler.  Less magic, more approachable, less 
> brittle with other tools (gradle, IDEs).  The original approach made lots of 
> sense in the original environment where Aurora was developed (a monorepo in a 
> large organization), but this is no longer the case.
> 
> I'm starting by collecting all options in a single class.  I chose the 
> jcommander library to parse args since it is amenable to having multiple 
> classes that hold args, which could be leveraged to bundle args into separate 
> classes if we choose to do so.
> 
> Historical context: 
> https://mail-archives.apache.org/mod_mbox/aurora-dev/201601.mbox/%3CCAFWq12VJPOxDViDP5OsmXU3skHR6rrp5Zp%2BXgi8HgeHic6pTJw%40mail.gmail.com%3E
> 
> 
> Diffs
> -
> 
>   build.gradle 460500aaeab28dcfeb29ec602d057ea4bca12378 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 4da91591d325ab657dcd37325e5142c65db2ab8c 
>   commons-args/src/main/java/org/apache/aurora/common/args/ArgParser.java 
> 25ed25093bd9defd78df741b6f51e0de0d1f1709 
>   commons-args/src/main/java/org/apache/aurora/common/args/CmdLine.java 
> a72ea7a1669a53d282f9a5a3709add506c9d4b33 
>   commons-args/src/main/java/org/apache/aurora/common/args/NoParser.java 
> 3366531cebe6a79d9c568322f9117d7dbc3e824d 
>   commons-args/src/main/java/org/apache/aurora/common/args/Parser.java 
> 93c23234df38c9f917c0f582b51c25070f996c94 
>   commons-args/src/main/java/org/apache/aurora/common/args/ParserOracle.java 
> e84d3029a45cf7ea1f0bb885788c677ed649b60e 
>   commons-args/src/main/java/org/apache/aurora/common/args/Verifier.java 
> 8f09a9803b8a05b2ab11326d48f26cccaaa278d1 
>   commons-args/src/main/java/org/apache/aurora/common/args/VerifierFor.java 
> aad8807938f569d0dc7a970166ee71ea36f3537d 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/CmdLineProcessor.java
>  5fda5dc6cd8b6d97511073830278850d58bb0bc7 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/Configuration.java
>  1832d41feeafb07f4e7f0bef9dbcb6097e288508 
>   
> commons-args/src/main/resources/META-INF/services/javax.annotation.processing.Processor
>  b548fcdc4389af4420e57908a313435b5300445f 
>   commons/src/main/java/org/apache/aurora/common/args/ArgFilters.java 
> 

Re: Review Request 62623: Use a simpler command line argument system

[Bill Farner]

> On Sept. 28, 2017, 10:15 p.m., Santhosh Kumar Shanmugham wrote:
> > Mostly nits around parameter validation.
> 
> Bill Farner wrote:
> My apologies, i could have been more clear on the _not ready to commit_ 
> line.  The commit needs a lot of polish, including parsers.  At this point 
> i'm looking for feedback for the shape of things.  Thanks for the thorough 
> look either way!
> 
> Santhosh Kumar Shanmugham wrote:
> Ah, my bad I missed the note.
> 
> +1 for removing commandline parsing code and consolidating the options in 
> a single place. Makes it much easier to traverse code.
> 
> However, what was the motivation for picking `JCommander`. Have you 
> considered `JOpt` or plain old `Jakarta-cli`? I ask since I find `JCommander` 
> to be limited.

> what was the motivation for picking JCommander

I like that JCommander requires explicit and up-front specificiation of args, 
types, and parsers.  This in contrast to getopt-style parsers that require 
calling code to fetch args by a name string, and deal with types and parsing at 
the call site.  Since we have over 100 args, we would invariably end up wanting 
to abstract away and centralize such behavior, ultimately finding ourselves 
writing another args lib.

> Have you considered JOpt or plain old Jakarta-cli

I discarded [JOpt Simple](https://pholser.github.io/jopt-simple/) and 
[commons-cli](https://commons.apache.org/proper/commons-cli/) (i assume that's 
what you mean by Jakarta-cli?) because they are both getopt-style libraries.


> On Sept. 28, 2017, 10:15 p.m., Santhosh Kumar Shanmugham wrote:
> > src/main/java/org/apache/aurora/scheduler/log/mesos/MesosLogStreamModule.java
> > Line 55 (original), 54 (patched)
> > 
> >
> > Can we delegate this constraint checking to `JCommander`?
> 
> Bill Farner wrote:
> I've found this tends to be awkward (in all arg libs i've seen) for args 
> that are required based on the value of other args, which is the case here.
> 
> Santhosh Kumar Shanmugham wrote:
> Sounds like we need a library with subcommand support.

JCommander does support subcommands, but i don't see how that helps here.  A 
subcommand is a different application entrypoint (do thing X _or_ do thing Y).  
Here we have `--enable-feature-y` and `--required-flag-for-feature-y`.


- Bill


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


On Sept. 27, 2017, 9:22 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62623/
> ---
> 
> (Updated Sept. 27, 2017, 9:22 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> **NOTE: this patch is not ready to commit, but is ready for initial 
> discussion on the direction**
> 
> This is a very big patch, lots of code removed.  I suggest starting with 
> `Options.java` on page 1, then looking at a few module classes to see the 
> call site changes.
> 
> I made a similar effort a while back, but was unable to finish due to time 
> constraints.  I'm now proposing a more drastic simplification in command line 
> argument handling in the scheduler.  Less magic, more approachable, less 
> brittle with other tools (gradle, IDEs).  The original approach made lots of 
> sense in the original environment where Aurora was developed (a monorepo in a 
> large organization), but this is no longer the case.
> 
> I'm starting by collecting all options in a single class.  I chose the 
> jcommander library to parse args since it is amenable to having multiple 
> classes that hold args, which could be leveraged to bundle args into separate 
> classes if we choose to do so.
> 
> Historical context: 
> https://mail-archives.apache.org/mod_mbox/aurora-dev/201601.mbox/%3CCAFWq12VJPOxDViDP5OsmXU3skHR6rrp5Zp%2BXgi8HgeHic6pTJw%40mail.gmail.com%3E
> 
> 
> Diffs
> -
> 
>   build.gradle 460500aaeab28dcfeb29ec602d057ea4bca12378 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 4da91591d325ab657dcd37325e5142c65db2ab8c 
>   commons-args/src/main/java/org/apache/aurora/common/args/ArgParser.java 
> 25ed25093bd9defd78df741b6f51e0de0d1f1709 
>   commons-args/src/main/java/org/apache/aurora/common/args/CmdLine.java 
> a72ea7a1669a53d282f9a5a3709add506c9d4b33 
>   commons-args/src/main/java/org/apache/aurora/common/args/NoParser.java 
> 3366531cebe6a79d9c568322f9117d7dbc3e824d 
>   commons-args/src/main/java/org/apache/aurora/common/args/Parser.java 
> 93c23234df38c9f917c0f582b51c25070f996c94 
>   

Re: Review Request 62623: Use a simpler command line argument system

[Jordan Ly]

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



I am definitely +1 to the spirit of this change.

I would prefer to go the route of multiple classes that hold arguments in 
logical groupings (ex. ReplicatedLogArgs and etc. along those lines). Do any 
modules in the scheduler share arguments? How is that done today? I am not too 
familiar with JCommander/argument parsing libraries but I was wondering how 
much functionality it gives out of the box for us right now vs. how many custom 
parsers we will have to write.

- Jordan Ly


On Sept. 28, 2017, 4:22 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62623/
> ---
> 
> (Updated Sept. 28, 2017, 4:22 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> **NOTE: this patch is not ready to commit, but is ready for initial 
> discussion on the direction**
> 
> This is a very big patch, lots of code removed.  I suggest starting with 
> `Options.java` on page 1, then looking at a few module classes to see the 
> call site changes.
> 
> I made a similar effort a while back, but was unable to finish due to time 
> constraints.  I'm now proposing a more drastic simplification in command line 
> argument handling in the scheduler.  Less magic, more approachable, less 
> brittle with other tools (gradle, IDEs).  The original approach made lots of 
> sense in the original environment where Aurora was developed (a monorepo in a 
> large organization), but this is no longer the case.
> 
> I'm starting by collecting all options in a single class.  I chose the 
> jcommander library to parse args since it is amenable to having multiple 
> classes that hold args, which could be leveraged to bundle args into separate 
> classes if we choose to do so.
> 
> Historical context: 
> https://mail-archives.apache.org/mod_mbox/aurora-dev/201601.mbox/%3CCAFWq12VJPOxDViDP5OsmXU3skHR6rrp5Zp%2BXgi8HgeHic6pTJw%40mail.gmail.com%3E
> 
> 
> Diffs
> -
> 
>   build.gradle 460500aaeab28dcfeb29ec602d057ea4bca12378 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 4da91591d325ab657dcd37325e5142c65db2ab8c 
>   commons-args/src/main/java/org/apache/aurora/common/args/ArgParser.java 
> 25ed25093bd9defd78df741b6f51e0de0d1f1709 
>   commons-args/src/main/java/org/apache/aurora/common/args/CmdLine.java 
> a72ea7a1669a53d282f9a5a3709add506c9d4b33 
>   commons-args/src/main/java/org/apache/aurora/common/args/NoParser.java 
> 3366531cebe6a79d9c568322f9117d7dbc3e824d 
>   commons-args/src/main/java/org/apache/aurora/common/args/Parser.java 
> 93c23234df38c9f917c0f582b51c25070f996c94 
>   commons-args/src/main/java/org/apache/aurora/common/args/ParserOracle.java 
> e84d3029a45cf7ea1f0bb885788c677ed649b60e 
>   commons-args/src/main/java/org/apache/aurora/common/args/Verifier.java 
> 8f09a9803b8a05b2ab11326d48f26cccaaa278d1 
>   commons-args/src/main/java/org/apache/aurora/common/args/VerifierFor.java 
> aad8807938f569d0dc7a970166ee71ea36f3537d 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/CmdLineProcessor.java
>  5fda5dc6cd8b6d97511073830278850d58bb0bc7 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/Configuration.java
>  1832d41feeafb07f4e7f0bef9dbcb6097e288508 
>   
> commons-args/src/main/resources/META-INF/services/javax.annotation.processing.Processor
>  b548fcdc4389af4420e57908a313435b5300445f 
>   commons/src/main/java/org/apache/aurora/common/args/ArgFilters.java 
> 9fd6eaec98516c3ca9b9a8647af848fcc96509bd 
>   commons/src/main/java/org/apache/aurora/common/args/ArgScanner.java 
> 6e7f23d79deff4983b7feb568320cb938583270b 
>   commons/src/main/java/org/apache/aurora/common/args/Args.java 
> 202835debce817df82bd3d860330d23d9710f489 
>   commons/src/main/java/org/apache/aurora/common/args/ArgumentInfo.java 
> a59d1091b36d0b7908143335cf49d0dafb6627a1 
>   commons/src/main/java/org/apache/aurora/common/args/OptionInfo.java 
> 2fcd3e8f068c4ef950aabe52ca8050dcbd562500 
>   commons/src/main/java/org/apache/aurora/common/args/Parsers.java 
> c4e5fafc49028bd15cfe1bd2a874e73792091cb6 
>   commons/src/main/java/org/apache/aurora/common/args/TypeUtil.java 
> 80cbdd00ce2abb298232793c5fd20c83c6ba6de4 
>   commons/src/main/java/org/apache/aurora/common/args/Verifiers.java 
> 0212873258867ccdb17244e7b0678a7346e79b73 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanExecute.java
>  a26b8a2049a16e96fd97ad2163556de41ba5686e 
>   
> commons/src/main/java/org/apache/aurora/common/args/constraints/CanExecuteFileVerifier.java
>  5d9b36070af7c717af5edcc0eec6774c2e26c102 
>   
> 

Re: Review Request 62623: Use a simpler command line argument system

[Santhosh Kumar Shanmugham]

> On Sept. 28, 2017, 10:15 p.m., Santhosh Kumar Shanmugham wrote:
> > Mostly nits around parameter validation.
> 
> Bill Farner wrote:
> My apologies, i could have been more clear on the _not ready to commit_ 
> line.  The commit needs a lot of polish, including parsers.  At this point 
> i'm looking for feedback for the shape of things.  Thanks for the thorough 
> look either way!

Ah, my bad I missed the note.

+1 for removing commandline parsing code and consolidating the options in a 
single place. Makes it much easier to traverse code.

However, what was the motivation for picking `JCommander`. Have you considered 
`JOpt` or plain old `Jakarta-cli`? I ask since I find `JCommander` to be 
limited.


> On Sept. 28, 2017, 10:15 p.m., Santhosh Kumar Shanmugham wrote:
> > src/main/java/org/apache/aurora/scheduler/log/mesos/MesosLogStreamModule.java
> > Line 55 (original), 54 (patched)
> > 
> >
> > Can we delegate this constraint checking to `JCommander`?
> 
> Bill Farner wrote:
> I've found this tends to be awkward (in all arg libs i've seen) for args 
> that are required based on the value of other args, which is the case here.

Sounds like we need a library with subcommand support.


- Santhosh Kumar


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


On Sept. 27, 2017, 9:22 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62623/
> ---
> 
> (Updated Sept. 27, 2017, 9:22 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> **NOTE: this patch is not ready to commit, but is ready for initial 
> discussion on the direction**
> 
> This is a very big patch, lots of code removed.  I suggest starting with 
> `Options.java` on page 1, then looking at a few module classes to see the 
> call site changes.
> 
> I made a similar effort a while back, but was unable to finish due to time 
> constraints.  I'm now proposing a more drastic simplification in command line 
> argument handling in the scheduler.  Less magic, more approachable, less 
> brittle with other tools (gradle, IDEs).  The original approach made lots of 
> sense in the original environment where Aurora was developed (a monorepo in a 
> large organization), but this is no longer the case.
> 
> I'm starting by collecting all options in a single class.  I chose the 
> jcommander library to parse args since it is amenable to having multiple 
> classes that hold args, which could be leveraged to bundle args into separate 
> classes if we choose to do so.
> 
> Historical context: 
> https://mail-archives.apache.org/mod_mbox/aurora-dev/201601.mbox/%3CCAFWq12VJPOxDViDP5OsmXU3skHR6rrp5Zp%2BXgi8HgeHic6pTJw%40mail.gmail.com%3E
> 
> 
> Diffs
> -
> 
>   build.gradle 460500aaeab28dcfeb29ec602d057ea4bca12378 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 4da91591d325ab657dcd37325e5142c65db2ab8c 
>   commons-args/src/main/java/org/apache/aurora/common/args/ArgParser.java 
> 25ed25093bd9defd78df741b6f51e0de0d1f1709 
>   commons-args/src/main/java/org/apache/aurora/common/args/CmdLine.java 
> a72ea7a1669a53d282f9a5a3709add506c9d4b33 
>   commons-args/src/main/java/org/apache/aurora/common/args/NoParser.java 
> 3366531cebe6a79d9c568322f9117d7dbc3e824d 
>   commons-args/src/main/java/org/apache/aurora/common/args/Parser.java 
> 93c23234df38c9f917c0f582b51c25070f996c94 
>   commons-args/src/main/java/org/apache/aurora/common/args/ParserOracle.java 
> e84d3029a45cf7ea1f0bb885788c677ed649b60e 
>   commons-args/src/main/java/org/apache/aurora/common/args/Verifier.java 
> 8f09a9803b8a05b2ab11326d48f26cccaaa278d1 
>   commons-args/src/main/java/org/apache/aurora/common/args/VerifierFor.java 
> aad8807938f569d0dc7a970166ee71ea36f3537d 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/CmdLineProcessor.java
>  5fda5dc6cd8b6d97511073830278850d58bb0bc7 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/Configuration.java
>  1832d41feeafb07f4e7f0bef9dbcb6097e288508 
>   
> commons-args/src/main/resources/META-INF/services/javax.annotation.processing.Processor
>  b548fcdc4389af4420e57908a313435b5300445f 
>   commons/src/main/java/org/apache/aurora/common/args/ArgFilters.java 
> 9fd6eaec98516c3ca9b9a8647af848fcc96509bd 
>   commons/src/main/java/org/apache/aurora/common/args/ArgScanner.java 
> 6e7f23d79deff4983b7feb568320cb938583270b 
>   commons/src/main/java/org/apache/aurora/common/args/Args.java 
> 202835debce817df82bd3d860330d23d9710f489 
>   

Re: Review Request 62623: Use a simpler command line argument system

[Bill Farner]

> On Sept. 29, 2017, 3:28 a.m., Reza Motamedi wrote:
> > src/main/java/org/apache/aurora/scheduler/config/Options.java
> > Lines 15 (patched)
> > 
> >
> > This looks a lot nicer. My question is if we are going enforce any kind 
> > of ordering or grouping of args, viz based on the class they are used in 
> > and such.
> > 
> > Should we just append to the end of this as they come along?

> My question is if we are going enforce any kind of ordering or grouping of 
> args, viz based on the class they are used in and such

Based on usage seems most sane.  Beyond that, i'd like to see args split out 
into individual classes for a more clear/natural grouping.  I held back on 
that, however, as i think the static read/write of options is a more pressing 
issue to address.


> On Sept. 29, 2017, 3:28 a.m., Reza Motamedi wrote:
> > src/main/java/org/apache/aurora/scheduler/config/Options.java
> > Lines 305 (patched)
> > 
> >
> > If we are validating every arg, should there be an IP validator as well?

Ideally, yes.  However, i'd rather not bog this change down by adding behavior 
that did not previously exist.


- Bill


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


On Sept. 27, 2017, 9:22 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62623/
> ---
> 
> (Updated Sept. 27, 2017, 9:22 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> **NOTE: this patch is not ready to commit, but is ready for initial 
> discussion on the direction**
> 
> This is a very big patch, lots of code removed.  I suggest starting with 
> `Options.java` on page 1, then looking at a few module classes to see the 
> call site changes.
> 
> I made a similar effort a while back, but was unable to finish due to time 
> constraints.  I'm now proposing a more drastic simplification in command line 
> argument handling in the scheduler.  Less magic, more approachable, less 
> brittle with other tools (gradle, IDEs).  The original approach made lots of 
> sense in the original environment where Aurora was developed (a monorepo in a 
> large organization), but this is no longer the case.
> 
> I'm starting by collecting all options in a single class.  I chose the 
> jcommander library to parse args since it is amenable to having multiple 
> classes that hold args, which could be leveraged to bundle args into separate 
> classes if we choose to do so.
> 
> Historical context: 
> https://mail-archives.apache.org/mod_mbox/aurora-dev/201601.mbox/%3CCAFWq12VJPOxDViDP5OsmXU3skHR6rrp5Zp%2BXgi8HgeHic6pTJw%40mail.gmail.com%3E
> 
> 
> Diffs
> -
> 
>   build.gradle 460500aaeab28dcfeb29ec602d057ea4bca12378 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 4da91591d325ab657dcd37325e5142c65db2ab8c 
>   commons-args/src/main/java/org/apache/aurora/common/args/ArgParser.java 
> 25ed25093bd9defd78df741b6f51e0de0d1f1709 
>   commons-args/src/main/java/org/apache/aurora/common/args/CmdLine.java 
> a72ea7a1669a53d282f9a5a3709add506c9d4b33 
>   commons-args/src/main/java/org/apache/aurora/common/args/NoParser.java 
> 3366531cebe6a79d9c568322f9117d7dbc3e824d 
>   commons-args/src/main/java/org/apache/aurora/common/args/Parser.java 
> 93c23234df38c9f917c0f582b51c25070f996c94 
>   commons-args/src/main/java/org/apache/aurora/common/args/ParserOracle.java 
> e84d3029a45cf7ea1f0bb885788c677ed649b60e 
>   commons-args/src/main/java/org/apache/aurora/common/args/Verifier.java 
> 8f09a9803b8a05b2ab11326d48f26cccaaa278d1 
>   commons-args/src/main/java/org/apache/aurora/common/args/VerifierFor.java 
> aad8807938f569d0dc7a970166ee71ea36f3537d 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/CmdLineProcessor.java
>  5fda5dc6cd8b6d97511073830278850d58bb0bc7 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/Configuration.java
>  1832d41feeafb07f4e7f0bef9dbcb6097e288508 
>   
> commons-args/src/main/resources/META-INF/services/javax.annotation.processing.Processor
>  b548fcdc4389af4420e57908a313435b5300445f 
>   commons/src/main/java/org/apache/aurora/common/args/ArgFilters.java 
> 9fd6eaec98516c3ca9b9a8647af848fcc96509bd 
>   commons/src/main/java/org/apache/aurora/common/args/ArgScanner.java 
> 6e7f23d79deff4983b7feb568320cb938583270b 
>   commons/src/main/java/org/apache/aurora/common/args/Args.java 
> 202835debce817df82bd3d860330d23d9710f489 
>   

Re: Review Request 62623: Use a simpler command line argument system

[Bill Farner]

> On Sept. 28, 2017, 10:15 p.m., Santhosh Kumar Shanmugham wrote:
> > Mostly nits around parameter validation.

My apologies, i could have been more clear on the _not ready to commit_ line.  
The commit needs a lot of polish, including parsers.  At this point i'm looking 
for feedback for the shape of things.  Thanks for the thorough look either way!


> On Sept. 28, 2017, 10:15 p.m., Santhosh Kumar Shanmugham wrote:
> > src/main/java/org/apache/aurora/scheduler/log/mesos/MesosLogStreamModule.java
> > Line 55 (original), 54 (patched)
> > 
> >
> > Can we delegate this constraint checking to `JCommander`?

I've found this tends to be awkward (in all arg libs i've seen) for args that 
are required based on the value of other args, which is the case here.


- Bill


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


On Sept. 27, 2017, 9:22 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62623/
> ---
> 
> (Updated Sept. 27, 2017, 9:22 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> **NOTE: this patch is not ready to commit, but is ready for initial 
> discussion on the direction**
> 
> This is a very big patch, lots of code removed.  I suggest starting with 
> `Options.java` on page 1, then looking at a few module classes to see the 
> call site changes.
> 
> I made a similar effort a while back, but was unable to finish due to time 
> constraints.  I'm now proposing a more drastic simplification in command line 
> argument handling in the scheduler.  Less magic, more approachable, less 
> brittle with other tools (gradle, IDEs).  The original approach made lots of 
> sense in the original environment where Aurora was developed (a monorepo in a 
> large organization), but this is no longer the case.
> 
> I'm starting by collecting all options in a single class.  I chose the 
> jcommander library to parse args since it is amenable to having multiple 
> classes that hold args, which could be leveraged to bundle args into separate 
> classes if we choose to do so.
> 
> Historical context: 
> https://mail-archives.apache.org/mod_mbox/aurora-dev/201601.mbox/%3CCAFWq12VJPOxDViDP5OsmXU3skHR6rrp5Zp%2BXgi8HgeHic6pTJw%40mail.gmail.com%3E
> 
> 
> Diffs
> -
> 
>   build.gradle 460500aaeab28dcfeb29ec602d057ea4bca12378 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 4da91591d325ab657dcd37325e5142c65db2ab8c 
>   commons-args/src/main/java/org/apache/aurora/common/args/ArgParser.java 
> 25ed25093bd9defd78df741b6f51e0de0d1f1709 
>   commons-args/src/main/java/org/apache/aurora/common/args/CmdLine.java 
> a72ea7a1669a53d282f9a5a3709add506c9d4b33 
>   commons-args/src/main/java/org/apache/aurora/common/args/NoParser.java 
> 3366531cebe6a79d9c568322f9117d7dbc3e824d 
>   commons-args/src/main/java/org/apache/aurora/common/args/Parser.java 
> 93c23234df38c9f917c0f582b51c25070f996c94 
>   commons-args/src/main/java/org/apache/aurora/common/args/ParserOracle.java 
> e84d3029a45cf7ea1f0bb885788c677ed649b60e 
>   commons-args/src/main/java/org/apache/aurora/common/args/Verifier.java 
> 8f09a9803b8a05b2ab11326d48f26cccaaa278d1 
>   commons-args/src/main/java/org/apache/aurora/common/args/VerifierFor.java 
> aad8807938f569d0dc7a970166ee71ea36f3537d 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/CmdLineProcessor.java
>  5fda5dc6cd8b6d97511073830278850d58bb0bc7 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/Configuration.java
>  1832d41feeafb07f4e7f0bef9dbcb6097e288508 
>   
> commons-args/src/main/resources/META-INF/services/javax.annotation.processing.Processor
>  b548fcdc4389af4420e57908a313435b5300445f 
>   commons/src/main/java/org/apache/aurora/common/args/ArgFilters.java 
> 9fd6eaec98516c3ca9b9a8647af848fcc96509bd 
>   commons/src/main/java/org/apache/aurora/common/args/ArgScanner.java 
> 6e7f23d79deff4983b7feb568320cb938583270b 
>   commons/src/main/java/org/apache/aurora/common/args/Args.java 
> 202835debce817df82bd3d860330d23d9710f489 
>   commons/src/main/java/org/apache/aurora/common/args/ArgumentInfo.java 
> a59d1091b36d0b7908143335cf49d0dafb6627a1 
>   commons/src/main/java/org/apache/aurora/common/args/OptionInfo.java 
> 2fcd3e8f068c4ef950aabe52ca8050dcbd562500 
>   commons/src/main/java/org/apache/aurora/common/args/Parsers.java 
> c4e5fafc49028bd15cfe1bd2a874e73792091cb6 
>   commons/src/main/java/org/apache/aurora/common/args/TypeUtil.java 
> 80cbdd00ce2abb298232793c5fd20c83c6ba6de4 
>   

Re: Review Request 62623: Use a simpler command line argument system

[Reza Motamedi]

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




src/main/java/org/apache/aurora/scheduler/config/Options.java
Lines 15 (patched)


This looks a lot nicer. My question is if we are going enforce any kind of 
ordering or grouping of args, viz based on the class they are used in and such.

Should we just append to the end of this as they come along?



src/main/java/org/apache/aurora/scheduler/config/Options.java
Lines 305 (patched)


If we are validating every arg, should there be an IP validator as well?



src/main/java/org/apache/aurora/scheduler/config/Options.java
Lines 604 (patched)


positive


- Reza Motamedi


On Sept. 28, 2017, 4:22 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62623/
> ---
> 
> (Updated Sept. 28, 2017, 4:22 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> **NOTE: this patch is not ready to commit, but is ready for initial 
> discussion on the direction**
> 
> This is a very big patch, lots of code removed.  I suggest starting with 
> `Options.java` on page 1, then looking at a few module classes to see the 
> call site changes.
> 
> I made a similar effort a while back, but was unable to finish due to time 
> constraints.  I'm now proposing a more drastic simplification in command line 
> argument handling in the scheduler.  Less magic, more approachable, less 
> brittle with other tools (gradle, IDEs).  The original approach made lots of 
> sense in the original environment where Aurora was developed (a monorepo in a 
> large organization), but this is no longer the case.
> 
> I'm starting by collecting all options in a single class.  I chose the 
> jcommander library to parse args since it is amenable to having multiple 
> classes that hold args, which could be leveraged to bundle args into separate 
> classes if we choose to do so.
> 
> Historical context: 
> https://mail-archives.apache.org/mod_mbox/aurora-dev/201601.mbox/%3CCAFWq12VJPOxDViDP5OsmXU3skHR6rrp5Zp%2BXgi8HgeHic6pTJw%40mail.gmail.com%3E
> 
> 
> Diffs
> -
> 
>   build.gradle 460500aaeab28dcfeb29ec602d057ea4bca12378 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 4da91591d325ab657dcd37325e5142c65db2ab8c 
>   commons-args/src/main/java/org/apache/aurora/common/args/ArgParser.java 
> 25ed25093bd9defd78df741b6f51e0de0d1f1709 
>   commons-args/src/main/java/org/apache/aurora/common/args/CmdLine.java 
> a72ea7a1669a53d282f9a5a3709add506c9d4b33 
>   commons-args/src/main/java/org/apache/aurora/common/args/NoParser.java 
> 3366531cebe6a79d9c568322f9117d7dbc3e824d 
>   commons-args/src/main/java/org/apache/aurora/common/args/Parser.java 
> 93c23234df38c9f917c0f582b51c25070f996c94 
>   commons-args/src/main/java/org/apache/aurora/common/args/ParserOracle.java 
> e84d3029a45cf7ea1f0bb885788c677ed649b60e 
>   commons-args/src/main/java/org/apache/aurora/common/args/Verifier.java 
> 8f09a9803b8a05b2ab11326d48f26cccaaa278d1 
>   commons-args/src/main/java/org/apache/aurora/common/args/VerifierFor.java 
> aad8807938f569d0dc7a970166ee71ea36f3537d 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/CmdLineProcessor.java
>  5fda5dc6cd8b6d97511073830278850d58bb0bc7 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/Configuration.java
>  1832d41feeafb07f4e7f0bef9dbcb6097e288508 
>   
> commons-args/src/main/resources/META-INF/services/javax.annotation.processing.Processor
>  b548fcdc4389af4420e57908a313435b5300445f 
>   commons/src/main/java/org/apache/aurora/common/args/ArgFilters.java 
> 9fd6eaec98516c3ca9b9a8647af848fcc96509bd 
>   commons/src/main/java/org/apache/aurora/common/args/ArgScanner.java 
> 6e7f23d79deff4983b7feb568320cb938583270b 
>   commons/src/main/java/org/apache/aurora/common/args/Args.java 
> 202835debce817df82bd3d860330d23d9710f489 
>   commons/src/main/java/org/apache/aurora/common/args/ArgumentInfo.java 
> a59d1091b36d0b7908143335cf49d0dafb6627a1 
>   commons/src/main/java/org/apache/aurora/common/args/OptionInfo.java 
> 2fcd3e8f068c4ef950aabe52ca8050dcbd562500 
>   commons/src/main/java/org/apache/aurora/common/args/Parsers.java 
> c4e5fafc49028bd15cfe1bd2a874e73792091cb6 
>   commons/src/main/java/org/apache/aurora/common/args/TypeUtil.java 
> 80cbdd00ce2abb298232793c5fd20c83c6ba6de4 
>   commons/src/main/java/org/apache/aurora/common/args/Verifiers.java 
> 0212873258867ccdb17244e7b0678a7346e79b73 
>   
> 

Re: Review Request 62623: Use a simpler command line argument system

[Santhosh Kumar Shanmugham]

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



Mostly nits around parameter validation.


src/main/java/org/apache/aurora/scheduler/TierModule.java
Line 48 (original)


In JCommander we will require an implementation of `IValueValidator`.



src/main/java/org/apache/aurora/scheduler/config/Options.java
Lines 113 (patched)


validateWith = PositiveInteger.class



src/main/java/org/apache/aurora/scheduler/config/Options.java
Lines 117 (patched)


validateWith = PositiveInteger.class



src/main/java/org/apache/aurora/scheduler/config/Options.java
Lines 157 (patched)


validateWith = PositiveInteger.class



src/main/java/org/apache/aurora/scheduler/config/Options.java
Lines 162 (patched)


validateWith = PositiveInteger.class



src/main/java/org/apache/aurora/scheduler/config/Options.java
Lines 165 (patched)


validateWith = x implements IValueValidator?

Verify the file has read permissions.



src/main/java/org/apache/aurora/scheduler/config/Options.java
Lines 168 (patched)


Positive



src/main/java/org/apache/aurora/scheduler/config/Options.java
Lines 239 (patched)


Positive



src/main/java/org/apache/aurora/scheduler/config/Options.java
Lines 300 (patched)


Non-negative?



src/main/java/org/apache/aurora/scheduler/config/Options.java
Lines 407 (patched)


validateWith = x implements IValueValidator?

Verify the file has read permissions.



src/main/java/org/apache/aurora/scheduler/config/Options.java
Lines 467 (patched)


validateWith = PositiveInteger.class ?



src/main/java/org/apache/aurora/scheduler/config/Options.java
Lines 471-472 (patched)


validateWith = PositiveInteger.class ?



src/main/java/org/apache/aurora/scheduler/config/Options.java
Lines 550 (patched)


Positive



src/main/java/org/apache/aurora/scheduler/config/Options.java
Lines 554 (patched)


Positive



src/main/java/org/apache/aurora/scheduler/config/Options.java
Lines 568 (patched)


Positive



src/main/java/org/apache/aurora/scheduler/config/Options.java
Lines 572 (patched)


Positive



src/main/java/org/apache/aurora/scheduler/config/Options.java
Lines 577 (patched)


validateWith = x implements IValueValidator?

Verify the file exists and has read permissions.



src/main/java/org/apache/aurora/scheduler/config/Options.java
Lines 624 (patched)


Non-negative



src/main/java/org/apache/aurora/scheduler/config/Options.java
Lines 628 (patched)


Non-negative



src/main/java/org/apache/aurora/scheduler/config/Options.java
Lines 675 (patched)


Positive



src/main/java/org/apache/aurora/scheduler/config/Options.java
Lines 679 (patched)


Positive



src/main/java/org/apache/aurora/scheduler/config/Options.java
Lines 690 (patched)


Positive



src/main/java/org/apache/aurora/scheduler/config/Options.java
Lines 693 (patched)


Positive



src/main/java/org/apache/aurora/scheduler/config/Options.java
Lines 720 (patched)


validateWith = PositiveInteger.class



src/main/java/org/apache/aurora/scheduler/http/api/security/Kerberos5ShiroRealmModule.java
Lines 62-63 (original)


nit - This file seems more appropriate for these constants rather than 
moving them into `Options`.



src/main/java/org/apache/aurora/scheduler/log/mesos/MesosLogStreamModule.java
Line 55 (original), 54 (patched)


Can we delegate this constraint checking to `JCommander`?


- Santhosh Kumar Shanmugham


On Sept. 27, 2017, 9:22 p.m., 

Re: Review Request 62623: Use a simpler command line argument system

[Aurora ReviewBot]

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



Master (7c78519) is red with this patch.
  ./build-support/jenkins/build.sh

:processJmhResources NO-SOURCE
:jmhClasses
:checkstyleJmh
:checkstyleMain
:checkstyleTest
:findbugsJmh
:findbugsMain
:findbugsTest
:licenseJmh UP-TO-DATE
:licenseMain UP-TO-DATE
:licenseTest UP-TO-DATE
:license UP-TO-DATE
:pmdJmh
:pmdMain
:pmdTest
:test

org.apache.aurora.scheduler.mesos.MesosCallbackHandlerTest > 
testRescindBeforeAdd FAILED
java.lang.AssertionError at MesosCallbackHandlerTest.java:357
I0928 04:37:03.259 [ShutdownHook, SchedulerMain] Stopping scheduler services. 

1155 tests completed, 1 failed, 2 skipped
:test FAILED
:jacocoTestReport
Coverage report generated: 
file:///home/jenkins/jenkins-slave/workspace/AuroraBot/dist/reports/jacoco/test/html/index.html
:jacocoTestCoverageVerification

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':test'.
> There were failing tests. See the report at: 
> file:///home/jenkins/jenkins-slave/workspace/AuroraBot/dist/reports/tests/test/index.html

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug 
option to get more log output.

* Get more help at https://help.gradle.org

BUILD FAILED in 6m 15s
48 actionable tasks: 39 executed, 9 up-to-date


I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Sept. 28, 2017, 4:22 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62623/
> ---
> 
> (Updated Sept. 28, 2017, 4:22 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> **NOTE: this patch is not ready to commit, but is ready for initial 
> discussion on the direction**
> 
> This is a very big patch, lots of code removed.  I suggest starting with 
> `Options.java` on page 1, then looking at a few module classes to see the 
> call site changes.
> 
> I made a similar effort a while back, but was unable to finish due to time 
> constraints.  I'm now proposing a more drastic simplification in command line 
> argument handling in the scheduler.  Less magic, more approachable, less 
> brittle with other tools (gradle, IDEs).  The original approach made lots of 
> sense in the original environment where Aurora was developed (a monorepo in a 
> large organization), but this is no longer the case.
> 
> I'm starting by collecting all options in a single class.  I chose the 
> jcommander library to parse args since it is amenable to having multiple 
> classes that hold args, which could be leveraged to bundle args into separate 
> classes if we choose to do so.
> 
> Historical context: 
> https://mail-archives.apache.org/mod_mbox/aurora-dev/201601.mbox/%3CCAFWq12VJPOxDViDP5OsmXU3skHR6rrp5Zp%2BXgi8HgeHic6pTJw%40mail.gmail.com%3E
> 
> 
> Diffs
> -
> 
>   build.gradle 460500aaeab28dcfeb29ec602d057ea4bca12378 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 4da91591d325ab657dcd37325e5142c65db2ab8c 
>   commons-args/src/main/java/org/apache/aurora/common/args/ArgParser.java 
> 25ed25093bd9defd78df741b6f51e0de0d1f1709 
>   commons-args/src/main/java/org/apache/aurora/common/args/CmdLine.java 
> a72ea7a1669a53d282f9a5a3709add506c9d4b33 
>   commons-args/src/main/java/org/apache/aurora/common/args/NoParser.java 
> 3366531cebe6a79d9c568322f9117d7dbc3e824d 
>   commons-args/src/main/java/org/apache/aurora/common/args/Parser.java 
> 93c23234df38c9f917c0f582b51c25070f996c94 
>   commons-args/src/main/java/org/apache/aurora/common/args/ParserOracle.java 
> e84d3029a45cf7ea1f0bb885788c677ed649b60e 
>   commons-args/src/main/java/org/apache/aurora/common/args/Verifier.java 
> 8f09a9803b8a05b2ab11326d48f26cccaaa278d1 
>   commons-args/src/main/java/org/apache/aurora/common/args/VerifierFor.java 
> aad8807938f569d0dc7a970166ee71ea36f3537d 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/CmdLineProcessor.java
>  5fda5dc6cd8b6d97511073830278850d58bb0bc7 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/Configuration.java
>  1832d41feeafb07f4e7f0bef9dbcb6097e288508 
>   
> commons-args/src/main/resources/META-INF/services/javax.annotation.processing.Processor
>  b548fcdc4389af4420e57908a313435b5300445f 
>   commons/src/main/java/org/apache/aurora/common/args/ArgFilters.java 
> 9fd6eaec98516c3ca9b9a8647af848fcc96509bd 
>   commons/src/main/java/org/apache/aurora/common/args/ArgScanner.java 
> 6e7f23d79deff4983b7feb568320cb938583270b 
>   commons/src/main/java/org/apache/aurora/common/args/Args.java 
> 

Review Request 62623: Use a simpler command line argument system

[Bill Farner]

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

Review request for Aurora, David McLaughlin and John Sirois.


Repository: aurora


Description
---

**NOTE: this patch is not ready to commit, but is ready for initial discussion 
on the direction**

This is a very big patch, lots of code removed.  I suggest starting with 
`Options.java` on page 1, then looking at a few module classes to see the call 
site changes.

I made a similar effort a while back, but was unable to finish due to time 
constraints.  I'm now proposing a more drastic simplification in command line 
argument handling in the scheduler.  Less magic, more approachable, less 
brittle with other tools (gradle, IDEs).  The original approach made lots of 
sense in the original environment where Aurora was developed (a monorepo in a 
large organization), but this is no longer the case.

I'm starting by collecting all options in a single class.  I chose the 
jcommander library to parse args since it is amenable to having multiple 
classes that hold args, which could be leveraged to bundle args into separate 
classes if we choose to do so.

Historical context: 
https://mail-archives.apache.org/mod_mbox/aurora-dev/201601.mbox/%3CCAFWq12VJPOxDViDP5OsmXU3skHR6rrp5Zp%2BXgi8HgeHic6pTJw%40mail.gmail.com%3E


Diffs
-

  build.gradle 460500aaeab28dcfeb29ec602d057ea4bca12378 
  commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
4da91591d325ab657dcd37325e5142c65db2ab8c 
  commons-args/src/main/java/org/apache/aurora/common/args/ArgParser.java 
25ed25093bd9defd78df741b6f51e0de0d1f1709 
  commons-args/src/main/java/org/apache/aurora/common/args/CmdLine.java 
a72ea7a1669a53d282f9a5a3709add506c9d4b33 
  commons-args/src/main/java/org/apache/aurora/common/args/NoParser.java 
3366531cebe6a79d9c568322f9117d7dbc3e824d 
  commons-args/src/main/java/org/apache/aurora/common/args/Parser.java 
93c23234df38c9f917c0f582b51c25070f996c94 
  commons-args/src/main/java/org/apache/aurora/common/args/ParserOracle.java 
e84d3029a45cf7ea1f0bb885788c677ed649b60e 
  commons-args/src/main/java/org/apache/aurora/common/args/Verifier.java 
8f09a9803b8a05b2ab11326d48f26cccaaa278d1 
  commons-args/src/main/java/org/apache/aurora/common/args/VerifierFor.java 
aad8807938f569d0dc7a970166ee71ea36f3537d 
  
commons-args/src/main/java/org/apache/aurora/common/args/apt/CmdLineProcessor.java
 5fda5dc6cd8b6d97511073830278850d58bb0bc7 
  
commons-args/src/main/java/org/apache/aurora/common/args/apt/Configuration.java 
1832d41feeafb07f4e7f0bef9dbcb6097e288508 
  
commons-args/src/main/resources/META-INF/services/javax.annotation.processing.Processor
 b548fcdc4389af4420e57908a313435b5300445f 
  commons/src/main/java/org/apache/aurora/common/args/ArgFilters.java 
9fd6eaec98516c3ca9b9a8647af848fcc96509bd 
  commons/src/main/java/org/apache/aurora/common/args/ArgScanner.java 
6e7f23d79deff4983b7feb568320cb938583270b 
  commons/src/main/java/org/apache/aurora/common/args/Args.java 
202835debce817df82bd3d860330d23d9710f489 
  commons/src/main/java/org/apache/aurora/common/args/ArgumentInfo.java 
a59d1091b36d0b7908143335cf49d0dafb6627a1 
  commons/src/main/java/org/apache/aurora/common/args/OptionInfo.java 
2fcd3e8f068c4ef950aabe52ca8050dcbd562500 
  commons/src/main/java/org/apache/aurora/common/args/Parsers.java 
c4e5fafc49028bd15cfe1bd2a874e73792091cb6 
  commons/src/main/java/org/apache/aurora/common/args/TypeUtil.java 
80cbdd00ce2abb298232793c5fd20c83c6ba6de4 
  commons/src/main/java/org/apache/aurora/common/args/Verifiers.java 
0212873258867ccdb17244e7b0678a7346e79b73 
  
commons/src/main/java/org/apache/aurora/common/args/constraints/CanExecute.java 
a26b8a2049a16e96fd97ad2163556de41ba5686e 
  
commons/src/main/java/org/apache/aurora/common/args/constraints/CanExecuteFileVerifier.java
 5d9b36070af7c717af5edcc0eec6774c2e26c102 
  commons/src/main/java/org/apache/aurora/common/args/constraints/CanRead.java 
3fef6a98e4c926126ac93061dd3b32c39f131e3c 
  
commons/src/main/java/org/apache/aurora/common/args/constraints/CanReadFileVerifier.java
 8c26304733264b6670e56ab032d7b039f4dfdbfe 
  commons/src/main/java/org/apache/aurora/common/args/constraints/CanWrite.java 
c2beeeb73d475ef9d03e1fde12e6d55983af81ed 
  
commons/src/main/java/org/apache/aurora/common/args/constraints/CanWriteFileVerifier.java
 eac273868d72993bc3e83742398c050db2ca4aea 
  commons/src/main/java/org/apache/aurora/common/args/constraints/Exists.java 
217d10e561b175885d6bb2a058225935fd88b37a 
  
commons/src/main/java/org/apache/aurora/common/args/constraints/ExistsFileVerifier.java
 e79f54701dea77440b585034790cc4cee987b99a 
  
commons/src/main/java/org/apache/aurora/common/args/constraints/IsDirectory.java
 d909994937e5ab7f9e125bdf000c8a22c2f84734 
  
commons/src/main/java/org/apache/aurora/common/args/constraints/IsDirectoryFileVerifier.java