> On Sept. 2, 2015, 4:45 p.m., Bill Farner wrote: > > examples/vagrant/executors-config.json, lines 17-18 > > <https://reviews.apache.org/r/37818/diff/3/?file=1061919#file1061919line17> > > > > Can you omit this since it's blank?
Yep, was just leaving it there for now so I don't forget to put it in the documaenttion for this feature. > On Sept. 2, 2015, 4:45 p.m., Bill Farner wrote: > > examples/vagrant/executors-config.json, lines 4-7 > > <https://reviews.apache.org/r/37818/diff/3/?file=1061919#file1061919line4> > > > > 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). We can go ahead and do that. Kevin thought it should be the array way, but I'm not sure how strongly he feels about it. The extra code isn't too bad because we can use String.join() form Java 8. Either way, I'm fine with either. The single string may eliminate any suspicions that we're breaking something in a command when joining the array. > On Sept. 2, 2015, 4:45 p.m., Bill Farner wrote: > > examples/vagrant/executors-config.json, line 32 > > <https://reviews.apache.org/r/37818/diff/3/?file=1061919#file1061919line32> > > > > Use single quotes to avoid escaping: > > > > "'Hello World from Aurora!'" Done > On Sept. 2, 2015, 4:45 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java, lines > > 84-115 > > <https://reviews.apache.org/r/37818/diff/3/?file=1061924#file1061924line84> > > > > Please add an entry in /NEWS under 0.10.0 to inform people what args > > were removed, and what the replacement is. Done. > On Sept. 2, 2015, 4:45 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/base/CommandUtil.java, lines 71-73 > > <https://reviews.apache.org/r/37818/diff/3/?file=1061925#file1061925line71> > > > > remove Done > On Sept. 2, 2015, 4:45 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/base/CommandUtil.java, lines > > 107-108 > > <https://reviews.apache.org/r/37818/diff/3/?file=1061925#file1061925line107> > > > > 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. Done > On Sept. 2, 2015, 4:45 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoader.java, > > line 46 > > <https://reviews.apache.org/r/37818/diff/3/?file=1061926#file1061926line46> > > > > please add a javadoc for this class Done > On Sept. 2, 2015, 4:45 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoader.java, > > line 62 > > <https://reviews.apache.org/r/37818/diff/3/?file=1061926#file1061926line62> > > > > Save a few lines: > > > > ``` > > String config = Files.toString(configFile, StandardCharsets.UTF_8); > > ``` Awesome. Done > On Sept. 2, 2015, 4:45 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoader.java, > > line 79 > > <https://reviews.apache.org/r/37818/diff/3/?file=1061926#file1061926line79> > > > > remove the ```\n```, same with the string below Done and Done > On Sept. 2, 2015, 4:45 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java, line 210 > > <https://reviews.apache.org/r/37818/diff/3/?file=1061924#file1061924line210> > > > > throw Throwables.propagate(e); > > > > Otherwise the app will exit in a non-obvious way. Thanks for pointing it out. I meant to come back to it but ended up forgetting about it. > On Sept. 2, 2015, 4:45 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoader.java, > > line 87 > > <https://reviews.apache.org/r/37818/diff/3/?file=1061926#file1061926line87> > > > > 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 wrote: > Oh, the detail i neglected to mention - i suggest you then write code to > translate your config schema object into ExecutorSettings. This can provides > compile-time guarantees that we don't have with the current code, making it > easier to maintain. Can you expend on this? I don't have any experience with Jackson. I'm looking through what Maxim did and I'll definitely crib here and there but I can't find anything related to getting this done at compile time. - Renan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37818/#review97468 ----------------------------------------------------------- On Sept. 2, 2015, 2:35 a.m., Renan DelValle wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37818/ > ----------------------------------------------------------- > > (Updated Sept. 2, 2015, 2:35 a.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 > >
