> 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(...);
>     }
>     ```

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.


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

Reply via email to