----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37818/#review97468 -----------------------------------------------------------
examples/vagrant/executors-config.json (lines 4 - 7) <https://reviews.apache.org/r/37818/#comment153348> The code later converts this array into a single command string. I suggest we just make this a string here (and save some code down the line). examples/vagrant/executors-config.json (lines 17 - 18) <https://reviews.apache.org/r/37818/#comment153342> Can you omit this since it's blank? examples/vagrant/executors-config.json (line 32) <https://reviews.apache.org/r/37818/#comment153343> Use single quotes to avoid escaping: "'Hello World from Aurora!'" src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java <https://reviews.apache.org/r/37818/#comment153344> Please add an entry in /NEWS under 0.10.0 to inform people what args were removed, and what the replacement is. src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java (line 166) <https://reviews.apache.org/r/37818/#comment153345> throw Throwables.propagate(e); Otherwise the app will exit in a non-obvious way. src/main/java/org/apache/aurora/scheduler/base/CommandUtil.java (lines 61 - 63) <https://reviews.apache.org/r/37818/#comment153346> remove src/main/java/org/apache/aurora/scheduler/base/CommandUtil.java (lines 95 - 96) <https://reviews.apache.org/r/37818/#comment153347> I suggest removing this for now. There's a lot of ways fetching the URIs and executing commands can fail. For the sake of simplicity, let's allow them to fail and focus on providing good visibility into the failure. src/main/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoader.java (line 46) <https://reviews.apache.org/r/37818/#comment153349> please add a javadoc for this class src/main/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoader.java (line 62) <https://reviews.apache.org/r/37818/#comment153351> Save a few lines: ``` String config = Files.toString(configFile, StandardCharsets.UTF_8); ``` src/main/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoader.java (lines 66 - 71) <https://reviews.apache.org/r/37818/#comment153352> You might want to consider using jackson instead of json. We recently learned that gson will silently ignore unexpected json properties, which is not behavior we want for a config file. This doesn't change much for you, see pointers in comment below for an example. src/main/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoader.java (line 79) <https://reviews.apache.org/r/37818/#comment153350> remove the ```\n```, same with the string below src/main/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoader.java (line 87) <https://reviews.apache.org/r/37818/#comment153360> This code will be easier to maintain long-term with 2 goals in mind: - keep json-related code within this class (no annotations on ExecutorSettings) - avoid hand-rolling deserializers This sounds like more work, but it's not that bad. The major difference is that you'll create a class in this file that defines the JSON structure, and does so in a way that the json deserializer can work with. Maxim just did something very similar, which you can crib from: https://github.com/apache/aurora/blob/89da936f3d28743e307c7c4fed0bff6fead7ceca/src/main/java/org/apache/aurora/scheduler/SchedulerModule.java#L132-L147 https://github.com/apache/aurora/blob/89da936f3d28743e307c7c4fed0bff6fead7ceca/src/main/java/org/apache/aurora/scheduler/TierManager.java#L47-L55 - Bill Farner On Sept. 1, 2015, 7:35 p.m., Renan DelValle wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37818/ > ----------------------------------------------------------- > > (Updated Sept. 1, 2015, 7:35 p.m.) > > > Review request for Aurora. > > > Repository: aurora > > > Description > ------- > > This is the first stage in a series of patches to create support for custom > executors. In an effort to expedite the review process, I have decided to > break down my patch into multiple pieces that when/if commited won't break > the trunk. > > This patch includes the ability to load configuration from a JSON file. A > JSON example file is included in examples/vagrant/executors-config.json > > Command line arguments have been eliminated and moved over to the JSON file. > GSON is leveraged and does most of the work with the aid of a few custom > deserializers that were needed. > > Note that right now a global container mount that does not follow > specification will cause the scheduler to detect the error an exit early. It > is up for discussion if this is the desired behavior or if we should just > ignore said mount. > > > Diffs > ----- > > examples/vagrant/executors-config.json PRE-CREATION > examples/vagrant/upstart/aurora-scheduler-kerberos.conf > 4f43892723db4744db205ea7dd107e9e9ce9d5db > examples/vagrant/upstart/aurora-scheduler.conf > e909451892f117e9e6eb80994079661827a0914c > src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java > c210c0db07bb1f4b3f76668178dcd7e2de56a4ac > src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java > 197184b6edc0768d677636341b5737f262abdf7d > src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java > 8047622e206c9827e5cd8e40152a278d495bd0ff > src/main/java/org/apache/aurora/scheduler/base/CommandUtil.java > aa5ce8b2f14c7dbd0eae120018ee41387c26059f > > src/main/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoader.java > PRE-CREATION > src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java > b3c913892248e4a9a8111412307463985f5ca97f > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java > f6ba2c40aea555d3e0ab774218bfe08d7e1c984b > src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java > 6fad3344042dc6a75cdf74ce79d388fcd4fc9861 > src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java > 1a25924d789295c5950947f5e302e1d1fbec68f2 > src/test/java/org/apache/aurora/scheduler/base/CommandUtilTest.java > cd0295780d41bc4e914583f195b37eaed28a46dc > > src/test/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoaderTest.java > PRE-CREATION > > src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java > dddf7952d3f0e508cd736d5fb95e573267708d43 > src/test/java/org/apache/aurora/scheduler/mesos/TaskExecutors.java > d0987251b058988fcbfab16c1b138c37e0c5b8c6 > > src/test/resources/org/apache/aurora/scheduler/configuration/executor-settings-thermos-no-observer.json > PRE-CREATION > > src/test/resources/org/apache/aurora/scheduler/configuration/multiple-executor-example.json > PRE-CREATION > > src/test/resources/org/apache/aurora/scheduler/configuration/no-value-URI.json > PRE-CREATION > > src/test/resources/org/apache/aurora/scheduler/configuration/single-executor-example.json > PRE-CREATION > > Diff: https://reviews.apache.org/r/37818/diff/ > > > Testing > ------- > > ./build-support/jenkins/build.sh: directory sandbox failed but it may be a > flaky test > bash src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > > > Thanks, > > Renan DelValle > >
