> 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 > 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 > >
