> 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. > > Renan DelValle wrote: > 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. > > Bill Farner wrote: > Don't be thrown by the mention of compile time, i'm just referring to > refactor safety if/when the structures changed. The concise way to put it is > - use a different class for your config domain object. This way you don't > need to change ExecutorSettings to compensate for the fact that some code in > another part of the system is using a json mapper. > > Start by defining a class that matches your config schema: > ``` > class ExecutorConfig { > String command; > List<Resources> resource; > ... > } > > class Resource { > String value; // note - i would like to see this renamed to uri > boolean executable; > boolean extract; > boolean cache; > } > ... > ``` > (that's just a quick example, feel free to adjust as you see fit) > > Jackson is very similar to gson, there's just a behavior nuance that is > desirable. You'll use an `ObjectMapper` to parse the json to an > `ExecutorConfig`, then write a function to transform your `ExecutorConfig` > into `ExecutorSettings`. > > ``` > private static ExecutorSettings configToSettings(ExecutorConfig config) { > return new ExecutorSettings(...); > } > ``` > > Bill Farner wrote: > I took a quick stab at illustrating this, take a look here: > https://github.com/wfarner/aurora/commit/a53c215432b34ba95238121581375bde24b88a1e > > The remaining work is to complete filling out constructor fields: > > https://github.com/wfarner/aurora/blob/a53c215432b34ba95238121581375bde24b88a1e/src/main/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoader.java#L53-L61 > > This is really a matter of writing functions to translate from types in > `ExecutorConfig` to the corresponding types in `ExecutorSettings`. > > Depending on your bandwidth over the next few days, i may take over this > patch if you don't mind.
Thanks for the examples, in particular the entry transformer really helped speed things along. I've almost got this done, and I'm having an issue with using optionals, which in my opinion are necessary if we want to leave the defaults set on the URI resource configuration if nothing is specified. I.e: If executable is not in the schema for a URI, then its better to not set it than to set a default value, IMO. I've looked around and Jackson has support for them if we load a new module: https://github.com/FasterXML/jackson-datatype-jdk8 However, I can't find where the jackson dependecy is. And the fact the jackson pacakges still point to codehaus lead me to believe were using an old version. Here are some relevant files: https://github.com/rdelval/aurora/blob/AURORA-1376-1d/src/main/java/org/apache/aurora/scheduler/configuration/ExecutorConfiguration.java https://github.com/rdelval/aurora/blob/AURORA-1376-1d/src/main/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoader.java We're almost there. > 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). > > Renan DelValle wrote: > 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. > > Kevin Sweeney wrote: > Bill, I prefer the array mode as it's more explicit for dealing with > magic characters in shell commands (like " (){}$"). > > You can still explicitly do > > ``` > ["sh", "-c", "PATH=/opt/aurora/executor/bin:$PATH thermos"] > ``` > > When you prefer the old form, but this form will not automatically expand > environment variables or do other shell magic for you. > > Renan - can you update CommandUtil to use the `arguments`[1] field > instead of `shell` and `value` (or drop a TODO for a later patch)? Changing > back to `String.join` defeats part of the purpose of the array form here. > > [1] > https://github.com/apache/mesos/blob/master/include/mesos/mesos.proto#L408-L423 > > Bill Farner wrote: > Aha - the `arguments` field in mesos.proto was the missing detail for me. > Since mesos allows us to do the right thing here, we should definitely use > an array. > > Renan DelValle wrote: > I gave this a shot, and it works fine for no container tasks. But, when > running a Docker container job, since the $MESOS_SANDBOX string is no longer > automatically expanding, it breaks. Should we just create a new CommandInfo > builder for docker that sets "sh" as the value and "-c" as the first argument > or is this a bad idea? > > Bill Farner wrote: > Kevin - any opinion on this? This is what I came up with, not ideal by any means, but I think it's something we can address later: https://github.com/rdelval/aurora/blob/AURORA-1376-1d/src/main/java/org/apache/aurora/scheduler/base/CommandUtil.java - Renan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37818/#review97468 ----------------------------------------------------------- On Sept. 8, 2015, 4:32 p.m., Renan DelValle wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37818/ > ----------------------------------------------------------- > > (Updated Sept. 8, 2015, 4:32 p.m.) > > > Review request for Aurora and Bill Farner. > > > 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 > >
