> On Sept. 2, 2015, 9:45 a.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. > > Renan DelValle wrote: > 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. > > Bill Farner wrote: > Perhaps we should go a step further and try to directly parse > `ExecutorInfo`? That way we don't have to maintain parity between the > protobuf schema and our own. Jackson can't directly deserialize > protobuf-generated objects, but a quick search turned up this promising > project: https://github.com/HubSpot/jackson-datatype-protobuf > > The deps are defined in build.gradle. You're right about the old jackson > library (pulled in by an old jersey-json). However, the fact that they > changed packages plays in our favor - we can add an explicit dep on the new > jackson version and use the new one in this part of the code.
I put together a proof of concept of this idea here: https://github.com/wfarner/aurora/commit/04e1ff6ad566755823b624303cfd66541076785b You'll notice that this makes the json more verbose, but it becomes a 1:1 association with the protobuf message. - Bill ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37818/#review97468 ----------------------------------------------------------- On Sept. 8, 2015, 9:32 a.m., Renan DelValle wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37818/ > ----------------------------------------------------------- > > (Updated Sept. 8, 2015, 9:32 a.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 > >
