> 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) > > <https://reviews.apache.org/r/62623/diff/1/?file=1837704#file1837704line56> > > > > 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 > 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 > > commons/src/main/java/org/apache/aurora/common/args/constraints/NotEmptyIterableVerifier.java > 222bdb2a2b97eb910242bdb63e86a19072653d31 > > commons/src/main/java/org/apache/aurora/common/args/constraints/NotEmptyStringVerifier.java > 4384a97d72320488e27db24c25d615a0134ea098 > > commons/src/main/java/org/apache/aurora/common/args/constraints/NotNegative.java > 1c1573fff0201205bc54e13f8a5882b48be23fab > > commons/src/main/java/org/apache/aurora/common/args/constraints/NotNegativeNumberVerifier.java > 9309224535da742fc69954c1885e170f0ecd844b > > commons/src/main/java/org/apache/aurora/common/args/constraints/NotNull.java > 57936be4af81d9f2e50271c5f4a1d03cf4a3d038 > > commons/src/main/java/org/apache/aurora/common/args/constraints/NotNullVerifier.java > 53669865ae9c52ecc9baa06de35bec6d7db6ce5c > > commons/src/main/java/org/apache/aurora/common/args/constraints/Positive.java > 2c91f4a956df5504b5aca89d584ae02ff43364b9 > > commons/src/main/java/org/apache/aurora/common/args/constraints/PositiveNumberVerifier.java > 95f88573fb01e550213d481afec1107417a6dbba > commons/src/main/java/org/apache/aurora/common/args/constraints/Range.java > 71ceb6a415a2a418df1de47be5be3d4cc4c57bd9 > > commons/src/main/java/org/apache/aurora/common/args/constraints/RangeNumberVerifier.java > bec05230c6e5dbb74cfd1e565ec2b8df315c6413 > > commons/src/main/java/org/apache/aurora/common/args/parsers/AmountParser.java > 1eb42bb1ca9eb7831cd2922ef161e33845afe9d1 > > commons/src/main/java/org/apache/aurora/common/args/parsers/BooleanParser.java > 9ceb148dc806489ba1dfe88b39558dc85c38eb96 > commons/src/main/java/org/apache/aurora/common/args/parsers/ByteParser.java > 64ee99885e3485e8a83ff6439a3d864413ebc221 > > commons/src/main/java/org/apache/aurora/common/args/parsers/CharacterParser.java > c2363578a9a31a247064984f31de39be74daf63a > > commons/src/main/java/org/apache/aurora/common/args/parsers/ClassParser.java > 27f71a8701346845b170fef3c28178e41280aaec > commons/src/main/java/org/apache/aurora/common/args/parsers/DateParser.java > a2ee8f9505d2ea35a8c751c2fe2ec329203f0ac0 > > commons/src/main/java/org/apache/aurora/common/args/parsers/DoubleParser.java > 6232437c39fdfc0c7791ce50f8dfb4f970c63b91 > > commons/src/main/java/org/apache/aurora/common/args/parsers/DurationParser.java > 7bf55782e0b91ca71ddebc7e221c91fccce7af4b > commons/src/main/java/org/apache/aurora/common/args/parsers/EnumParser.java > 1c1e9bf1ed3d4c2ead0ab14187f3da96db04d650 > commons/src/main/java/org/apache/aurora/common/args/parsers/FileParser.java > c8573f530ccbbd37275d8a905425663a12458358 > > commons/src/main/java/org/apache/aurora/common/args/parsers/FloatParser.java > 7ceb11012fa94b199d122109760609312ddab260 > > commons/src/main/java/org/apache/aurora/common/args/parsers/InetSocketAddressParser.java > dfa441eb3236dbaeb9fa9ad69996532c0270b1b2 > > commons/src/main/java/org/apache/aurora/common/args/parsers/IntegerParser.java > 275d1212f56aa1e4033945ac5b2166f0e2b5cfda > commons/src/main/java/org/apache/aurora/common/args/parsers/ListParser.java > c22443450f22760d71bdb44ed5982a3bf2916ff5 > commons/src/main/java/org/apache/aurora/common/args/parsers/LongParser.java > 95f92b89d2a6d7165f3cf4279083d34571059a39 > commons/src/main/java/org/apache/aurora/common/args/parsers/MapParser.java > 3e1c9166ceb5fc9de94563ef84b46daef577c6e5 > > commons/src/main/java/org/apache/aurora/common/args/parsers/MultimapParser.java > 0c44614dc4bb0fc4e1493aa0f19114b6a00751e4 > > commons/src/main/java/org/apache/aurora/common/args/parsers/NonParameterizedTypeParser.java > c77543c5dd31a297f83e4f79be5491d1b9508b9d > > commons/src/main/java/org/apache/aurora/common/args/parsers/NumberParser.java > a551fa959050bf87c097413f5cbf4edde880c426 > commons/src/main/java/org/apache/aurora/common/args/parsers/PairParser.java > 3465260a635dd82f85506310d41208bd563d2bdb > > commons/src/main/java/org/apache/aurora/common/args/parsers/PatternParser.java > 0b86a21f5f44d97433847167041aba10400a732b > > commons/src/main/java/org/apache/aurora/common/args/parsers/RangeParser.java > e8616810a34bf048174eebece0b223048b0fba63 > commons/src/main/java/org/apache/aurora/common/args/parsers/SetParser.java > 15b6c745ada4fbd861f99f3d5b3a6aa535ac8e69 > > commons/src/main/java/org/apache/aurora/common/args/parsers/ShortParser.java > 1e1323dca05e4fcf25f885e5c94a247247324bcd > > commons/src/main/java/org/apache/aurora/common/args/parsers/StringParser.java > 5283992481c3c931c67b66d7512cfc25bbb431bc > > commons/src/main/java/org/apache/aurora/common/args/parsers/TypeParameterizedParser.java > a8455f5bae6c456b1fd4cc835b1f36f58f200728 > commons/src/main/java/org/apache/aurora/common/args/parsers/URIParser.java > 602611264474ea5277670f2b7f67c00785adf679 > commons/src/main/java/org/apache/aurora/common/args/parsers/URLParser.java > c2fd3014b0af11f813111b5c3e2358fe4515de20 > commons/src/main/java/org/apache/aurora/common/args/parsers/UnitParser.java > 91098530d697935e02155c208d181102ba63c15a > commons/src/test/java/org/apache/aurora/common/args/ArgFiltersTest.java > 03908090f8079413d585316162c6ae70e4e7d534 > commons/src/test/java/org/apache/aurora/common/args/ArgScannerTest.java > 06ce9144a838c69c61488f8a2c8a18e4b3284457 > commons/src/test/java/org/apache/aurora/common/args/ArgTest.java > cbcc575abf13c30984f14f6e828ac4d19b737184 > commons/src/test/java/org/apache/aurora/common/args/ArgsTest.java > 7bccababe87481ae417d7d2d3e91868f7732a7fc > commons/src/test/java/org/apache/aurora/common/args/OptionInfoTest.java > 75734305db8e608a75eafb5a4bbcfecf8303057d > commons/src/test/java/org/apache/aurora/common/args/ParsersTest.java > 991291de7e3c12dc234dc57e622a97358ff6731d > > commons/src/test/java/org/apache/aurora/common/args/argfilterstest/ArgsRoot.java > fefff0c43638d51630deb2ba6c4ba61ca1016295 > > commons/src/test/java/org/apache/aurora/common/args/argfilterstest/subpackageA/ArgsA.java > 7777875b450585800e4885a3bc5150a6f8755b0f > > commons/src/test/java/org/apache/aurora/common/args/argfilterstest/subpackageA/subsubpackage1/ArgsA1.java > 43af4f2882b0ebe9300f9ccf9733ae4418dd3673 > > commons/src/test/java/org/apache/aurora/common/args/argfilterstest/subpackageB/ArgsB.java > 11ed7e33ea85f1b0e3c62c348073d67924d1ef99 > > commons/src/test/java/org/apache/aurora/common/args/argfilterstest/subpackageBwithSuffix/ArgsBWithSuffix.java > 0ef8d5f92f2387ef1a687089a8e6d7376dfa52b8 > src/main/java/org/apache/aurora/scheduler/SchedulerModule.java > a62bb069493c41d0ddac783d26a92a9ee3ab8434 > src/main/java/org/apache/aurora/scheduler/TierModule.java > 61afa31bf975a580dfe8fa120015eea364f7d3f7 > src/main/java/org/apache/aurora/scheduler/app/AppModule.java > 081dff2bda626f01ba222628b8d7d8afebbb0004 > src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java > 3fbe99c4f1af1f11403bf08155bb4be028132e38 > src/main/java/org/apache/aurora/scheduler/app/VolumeParser.java > c1e99cee7a824b1440c9ef8c23c0d6834b6a1394 > src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java > da07df66b06cef6223119854032b4ca1c57a0859 > src/main/java/org/apache/aurora/scheduler/config/Options.java PRE-CREATION > > src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java > 4dac9757a65e144142d36ee921b85a02a5311fe5 > src/main/java/org/apache/aurora/scheduler/cron/quartz/CronModule.java > 9c88a2aa212b43d1762f5da816c5e99f32af79d3 > > src/main/java/org/apache/aurora/scheduler/discovery/FlaggedZooKeeperConfig.java > e8aafe4a47e3f0e6312c8b93dbc32e1c25445dd3 > src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java > 1f10af71830386652d21961b733bd0927c5436a1 > src/main/java/org/apache/aurora/scheduler/http/H2ConsoleModule.java > 01d6b5de0079d6f5709c29fe9a72829fbc8501de > src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java > e29bb411bcf399a85b8508f66ae8acc5b2294567 > src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java > e46820952fb6028911bca924169ceade6a134bfc > > src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java > 5bba496522a689e5de0ce5be58fe2bf9182966ce > > src/main/java/org/apache/aurora/scheduler/http/api/security/IniShiroRealmModule.java > 945846895c4aaad51f59f88b56bc6b9a865b45c1 > > src/main/java/org/apache/aurora/scheduler/http/api/security/Kerberos5ShiroRealmModule.java > 9c7aeadfecfca4c766a0430c7927eac5983c7cc8 > > src/main/java/org/apache/aurora/scheduler/http/api/security/KerberosPrincipalParser.java > bce20b7e15548cddab786acabd3ed7461d9db94a > > src/main/java/org/apache/aurora/scheduler/http/api/security/ModuleParser.java > ccd9a20e8b18831458cba2d53e6b8b84fef06162 > > src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroIniParser.java > 3ee41b81b7e30375e63f310ed44ce8a1381a6722 > > src/main/java/org/apache/aurora/scheduler/log/mesos/MesosLogStreamModule.java > 9a6c0c4782531d1ea6d0cce0251b31bab7c52440 > > src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java > 97d3c209f63d1fc76975e1a4d8193ace4e7cb7e5 > src/main/java/org/apache/aurora/scheduler/mesos/LibMesosLoadingModule.java > 3e943ff9ecd6e2de31c8121aa684d74256456a54 > src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java > 18dc3e033fad213d5ddc6b21c45c0b6dc9e0870d > src/main/java/org/apache/aurora/scheduler/offers/OffersModule.java > bbccb17f890ce701c4f199a688ab388c3be6b392 > src/main/java/org/apache/aurora/scheduler/preemptor/PreemptorModule.java > b3ca1a37308c876ccb0a5b0b31a182662c318ca6 > src/main/java/org/apache/aurora/scheduler/pruning/PruningModule.java > 735199ac1ccccab343c24471890aa330d6635c26 > > src/main/java/org/apache/aurora/scheduler/reconciliation/ReconciliationModule.java > 80fc6165186ffaf2c83b7f410d18ceb688720efe > src/main/java/org/apache/aurora/scheduler/resources/ResourceSettings.java > 4e2c42526e91acfbb2f75bcb99c8a8a9adb353d1 > src/main/java/org/apache/aurora/scheduler/resources/ResourceType.java > de43eaaf2881c5e84c09948deb9d37870588804d > src/main/java/org/apache/aurora/scheduler/scheduling/SchedulingModule.java > 04150165ac538ce95dfef9808f7927e7a1990158 > src/main/java/org/apache/aurora/scheduler/sla/MetricCalculator.java > 9a56cda809fbbcb07e6dd12c7a0feb272542491d > src/main/java/org/apache/aurora/scheduler/sla/SlaModule.java > d569241a59f169eaa9982c3bba7003aa4942f50f > src/main/java/org/apache/aurora/scheduler/state/StateModule.java > 77a37b8766e3b58151368ac11def805d10315786 > src/main/java/org/apache/aurora/scheduler/stats/AsyncStatsModule.java > 40451e91aed45866c2030d901160cc4e084834df > src/main/java/org/apache/aurora/scheduler/stats/StatsModule.java > 4767ef12e6a3c9d7b2d4a2b5be27786518b5b612 > src/main/java/org/apache/aurora/scheduler/storage/backup/BackupModule.java > cded40ba4981e0ae287b6a24e49523f40674bef9 > src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java > 0a2516e4843b8f920700ece70b3cc816d2acecf0 > src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java > 835f1604c0c5d913a87d570ee01d053bbbf92ecb > > src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java > 9d2164f4f2d18e4595d3039d64cedacb7df00ae2 > > src/main/java/org/apache/aurora/scheduler/storage/mem/InMemStoresModule.java > 1b491f977cf3a81e61f1333082be0547420306d4 > src/main/java/org/apache/aurora/scheduler/thrift/aop/AopModule.java > ef5edf614b0166ae209657abc81bf849af4817c5 > src/main/java/org/apache/aurora/scheduler/updater/UpdaterModule.java > 36730319265d02fe1a40701f9973e6dc2cb8b532 > src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java > a7c9c83eebbbea7ae8a6c807f98d3ce8bd050137 > src/test/java/org/apache/aurora/scheduler/app/VolumeParserTest.java > e8f8449b967f15a85219c2be57556db78f42f57f > > src/test/java/org/apache/aurora/scheduler/http/api/security/KerberosPrincipalParserTest.java > 7e55ef832c3cc36501cb08de389cf5931cb07c34 > > src/test/java/org/apache/aurora/scheduler/http/api/security/ModuleParserTest.java > baaeb2390a909de1a92e4328d35a49f7b74c36cb > > src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroIniParserTest.java > 3ca8c861a1e768d2a8361cd6d079a66ffce05ee5 > > src/test/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModuleTest.java > d2c829e56f4973333f35bdfc5387a35e33fe5440 > src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java > 6b4b17f8dafd5c2d751dcda3080b476335f8aec0 > > > Diff: https://reviews.apache.org/r/62623/diff/1/ > > > Testing > ------- > > > Thanks, > > Bill Farner > >
