Re: Review Request 37818: Moved executor settings configuration to loadable JSON

[Aurora ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37818/#review101247
---

Ship it!


Master (33d7e21) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Oct. 1, 2015, 5:38 p.m., Renan DelValle wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37818/
> ---
> 
> (Updated Oct. 1, 2015, 5:38 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. 
> Jackson is used to deserialize a CommandInfo.Builder along with two other 
> fields. Volumes are now based on the Protobuf version and not the Aurora 
> Thrift version.
> 
> 
> Diffs
> -
> 
>   NEWS 2edcea61fafea09b08faf03af4335ca82629d46d 
>   build.gradle 0401a9cbf0caf5578ef3c30e67c5b6a0a7a74a03 
>   examples/vagrant/executors-config.json PRE-CREATION 
>   examples/vagrant/upstart/aurora-scheduler-kerberos.conf 
> 4f43892723db4744db205ea7dd107e9e9ce9d5db 
>   examples/vagrant/upstart/aurora-scheduler.conf 
> 4033184451f36cb5f0233ea96e3dceaae6741275 
>   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 
> 99b874459c358c67234c6fe748e6427f313a04a8 
>   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 
> f63d6f12d114c062f81137ffc0b1078837e3cf76 
>   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/multiple-executor-example.json
>  PRE-CREATION 
>   
> src/test/resources/org/apache/aurora/scheduler/configuration/single-executor-example.json
>  PRE-CREATION 
>   
> src/test/resources/org/apache/aurora/scheduler/configuration/thermos-no-observer.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
> 
>

Re: Review Request 37818: Moved executor settings configuration to loadable JSON

[Renan DelValle]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37818/
---

(Updated Sept. 30, 2015, 9:10 p.m.)


Review request for Aurora and Bill Farner.


Changes
---

Fixed bug that was causing end to end tests to fail. Needed to add an arg0. 
Fixed docker creation, docker commandinfo's are still generated like they used 
to and run with shell as true due to the fact that an environmental var is 
needed to run the tasks.

@ReviewBot retry


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. 
Jackson is used to deserialize a CommandInfo.Builder along with two other 
fields. Volumes are now based on the Protobuf version and not the Aurora Thrift 
version.


Diffs (updated)
-

  NEWS 2edcea61fafea09b08faf03af4335ca82629d46d 
  build.gradle 0401a9cbf0caf5578ef3c30e67c5b6a0a7a74a03 
  examples/vagrant/executors-config.json PRE-CREATION 
  examples/vagrant/upstart/aurora-scheduler-kerberos.conf 
4f43892723db4744db205ea7dd107e9e9ce9d5db 
  examples/vagrant/upstart/aurora-scheduler.conf 
4033184451f36cb5f0233ea96e3dceaae6741275 
  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 
99b874459c358c67234c6fe748e6427f313a04a8 
  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 
f63d6f12d114c062f81137ffc0b1078837e3cf76 
  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/multiple-executor-example.json
 PRE-CREATION 
  
src/test/resources/org/apache/aurora/scheduler/configuration/single-executor-example.json
 PRE-CREATION 
  
src/test/resources/org/apache/aurora/scheduler/configuration/thermos-no-observer.json
 PRE-CREATION 

Diff: https://reviews.apache.org/r/37818/diff/


Testing (updated)
---

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

Re: Review Request 37818: Moved executor settings configuration to loadable JSON

[Aurora ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37818/#review101158
---

Ship it!


Master (488f29f) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Sept. 30, 2015, 9:10 p.m., Renan DelValle wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37818/
> ---
> 
> (Updated Sept. 30, 2015, 9:10 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. 
> Jackson is used to deserialize a CommandInfo.Builder along with two other 
> fields. Volumes are now based on the Protobuf version and not the Aurora 
> Thrift version.
> 
> 
> Diffs
> -
> 
>   NEWS 2edcea61fafea09b08faf03af4335ca82629d46d 
>   build.gradle 0401a9cbf0caf5578ef3c30e67c5b6a0a7a74a03 
>   examples/vagrant/executors-config.json PRE-CREATION 
>   examples/vagrant/upstart/aurora-scheduler-kerberos.conf 
> 4f43892723db4744db205ea7dd107e9e9ce9d5db 
>   examples/vagrant/upstart/aurora-scheduler.conf 
> 4033184451f36cb5f0233ea96e3dceaae6741275 
>   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 
> 99b874459c358c67234c6fe748e6427f313a04a8 
>   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 
> f63d6f12d114c062f81137ffc0b1078837e3cf76 
>   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/multiple-executor-example.json
>  PRE-CREATION 
>   
> src/test/resources/org/apache/aurora/scheduler/configuration/single-executor-example.json
>  PRE-CREATION 
>   
> src/test/resources/org/apache/aurora/scheduler/configuration/thermos-no-observer.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
> 
>

Re: Review Request 37818: Moved executor settings configuration to loadable JSON

[Renan DelValle]

> On Sept. 2, 2015, 4:45 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoader.java,
> >  line 87
> > 
> >
> > 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 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 

Re: Review Request 37818: Moved executor settings configuration to loadable JSON

[Aurora ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37818/#review99873
---


This patch does not apply cleanly on master (5627611), do you need to rebase?

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Sept. 21, 2015, 11:42 p.m., Renan DelValle wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37818/
> ---
> 
> (Updated Sept. 21, 2015, 11:42 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. 
> Jackson is used to deserialize a CommandInfo.Builder along with two other 
> fields. Volumes are now based on the Protobuf version and not the Aurora 
> Thrift version.
> 
> 
> Diffs
> -
> 
>   .gitignore 86840972c53ea52a793968d3d00df6763a7d6ffb 
>   3rdparty/python/requirements.txt 44217469a9583ec50233f34d54a32c105e6bab2c 
>   CHANGELOG 3b6990fb4dc3cfb8c730c81063413e505c803471 
>   NEWS a17f0e7c08fd30a0b2db6814a1c755111307228b 
>   README.md ef9c2f0ecf44b03e2025fe9cab62a3aa5ae30a77 
>   api/src/main/thrift/org/apache/aurora/gen/BUILD 
> fe3f83b6a7680985dce01efe2d54ccc4b0c2c482 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> d740a90e7732f42b43a79f8cf0afe705c061539c 
>   api/src/main/thrift/org/apache/aurora/gen/internal_rpc.thrift 
> a2c230fa9b5f648c4674042411cbe46fb8bb4faa 
>   api/src/main/thrift/org/apache/aurora/gen/storage.thrift 
> b7c4665d51f0efeebabd58c978707397b45d275d 
>   api/src/main/thrift/org/apache/thermos/BUILD 
> d0d789a6ee3971e3070f9397d53929563a77f7ea 
>   build-support/python/isort 44f9659948703c75372cd70643d5631acb116c2e 
>   build-support/python/isort-check 646cbf0bae4ccf8eac044817138ed1a7a59b261b 
>   build-support/python/make-mesos-native-egg 
> fce53e42dde72b893f291db9d78af1dc47fafe2f 
>   build-support/python/make-pycharm-virtualenv 
> 05a16d0d421982bcfb2e649be7b83a17d813efb1 
>   build-support/release/make-python-sdists 
> 9608f68e16243da01434ce2fc7d61bb7c7efd712 
>   build.gradle 700e1ade1470b99e9be390db150cf73aa06f17bc 
>   buildSrc/gradle.properties 3231a7add9e0503f0ee6779ae3c4000143b174ab 
>   buildSrc/src/main/groovy/org/apache/aurora/build/CoverageReportCheck.groovy 
> b996d4597492a76db0f2571db4e6c1ae9b4bcf33 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> PRE-CREATION 
>   commons-args/src/main/java/org/apache/aurora/common/args/ArgParser.java 
> PRE-CREATION 
>   commons-args/src/main/java/org/apache/aurora/common/args/CmdLine.java 
> PRE-CREATION 
>   commons-args/src/main/java/org/apache/aurora/common/args/NoParser.java 
> PRE-CREATION 
>   commons-args/src/main/java/org/apache/aurora/common/args/Parser.java 
> PRE-CREATION 
>   commons-args/src/main/java/org/apache/aurora/common/args/ParserOracle.java 
> PRE-CREATION 
>   commons-args/src/main/java/org/apache/aurora/common/args/Positional.java 
> PRE-CREATION 
>   commons-args/src/main/java/org/apache/aurora/common/args/Verifier.java 
> PRE-CREATION 
>   commons-args/src/main/java/org/apache/aurora/common/args/VerifierFor.java 
> PRE-CREATION 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/CmdLineProcessor.java
>  PRE-CREATION 
>   
> commons-args/src/main/java/org/apache/aurora/common/args/apt/Configuration.java
>  PRE-CREATION 
>   
> commons-args/src/main/resources/META-INF/services/javax.annotation.processing.Processor
>  PRE-CREATION 
>   commons/src/main/java/org/apache/aurora/common/application/Lifecycle.java 
> PRE-CREATION 
>   
> commons/src/main/java/org/apache/aurora/common/application/ShutdownRegistry.java
>  PRE-CREATION 
>   
> commons/src/main/java/org/apache/aurora/common/application/ShutdownStage.java 
> PRE-CREATION 
>   commons/src/main/java/org/apache/aurora/common/args/ArgFilters.java 
> PRE-CREATION 
>   commons/src/main/java/org/apache/aurora/common/args/ArgScanner.java 
> PRE-CREATION 
>   commons/src/main/java/org/apache/aurora/common/args/Args.java PRE-CREATION 
>   commons/src/main/java/org/apache/aurora/common/args/ArgumentInfo.java 
> PRE-CREATION 
>   commons/src/main/java/org/apache/aurora/common/args/OptionInfo.java 
> PRE-CREATION 
>   commons/src/main/java/org/apache/aurora/common/args/Parsers.java 
> 

Re: Review Request 37818: Moved executor settings configuration to loadable JSON

[Renan DelValle]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37818/
---

(Updated Sept. 21, 2015, 11:42 p.m.)


Review request for Aurora and Bill Farner.


Changes
---

Switched JSON parser from GSON to Jackson. 

Changed ExcutorSettings to contain CommandInfo.Builder and eliminated other 
redudant fields as a result.

Changed executors-config schema to be a serialized version of 
CommandInfo.Builder, allowing us to configure CommandInfo from the file as per 
the mesos protobuf protocol.

This patch failes the end to end test, I haven't been able to pinpoint the 
reason why the serverset fails to announce the job but my hope is that more 
eyes on the code can help me fix this faster.


Repository: aurora


Description (updated)
---

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. 
Jackson is used to deserialize a CommandInfo.Builder along with two other 
fields. Volumes are now based on the Protobuf version and not the Aurora Thrift 
version.


Diffs (updated)
-

  .gitignore 86840972c53ea52a793968d3d00df6763a7d6ffb 
  3rdparty/python/requirements.txt 44217469a9583ec50233f34d54a32c105e6bab2c 
  CHANGELOG 3b6990fb4dc3cfb8c730c81063413e505c803471 
  NEWS a17f0e7c08fd30a0b2db6814a1c755111307228b 
  README.md ef9c2f0ecf44b03e2025fe9cab62a3aa5ae30a77 
  api/src/main/thrift/org/apache/aurora/gen/BUILD 
fe3f83b6a7680985dce01efe2d54ccc4b0c2c482 
  api/src/main/thrift/org/apache/aurora/gen/api.thrift 
d740a90e7732f42b43a79f8cf0afe705c061539c 
  api/src/main/thrift/org/apache/aurora/gen/internal_rpc.thrift 
a2c230fa9b5f648c4674042411cbe46fb8bb4faa 
  api/src/main/thrift/org/apache/aurora/gen/storage.thrift 
b7c4665d51f0efeebabd58c978707397b45d275d 
  api/src/main/thrift/org/apache/thermos/BUILD 
d0d789a6ee3971e3070f9397d53929563a77f7ea 
  build-support/python/isort 44f9659948703c75372cd70643d5631acb116c2e 
  build-support/python/isort-check 646cbf0bae4ccf8eac044817138ed1a7a59b261b 
  build-support/python/make-mesos-native-egg 
fce53e42dde72b893f291db9d78af1dc47fafe2f 
  build-support/python/make-pycharm-virtualenv 
05a16d0d421982bcfb2e649be7b83a17d813efb1 
  build-support/release/make-python-sdists 
9608f68e16243da01434ce2fc7d61bb7c7efd712 
  build.gradle 700e1ade1470b99e9be390db150cf73aa06f17bc 
  buildSrc/gradle.properties 3231a7add9e0503f0ee6779ae3c4000143b174ab 
  buildSrc/src/main/groovy/org/apache/aurora/build/CoverageReportCheck.groovy 
b996d4597492a76db0f2571db4e6c1ae9b4bcf33 
  commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
PRE-CREATION 
  commons-args/src/main/java/org/apache/aurora/common/args/ArgParser.java 
PRE-CREATION 
  commons-args/src/main/java/org/apache/aurora/common/args/CmdLine.java 
PRE-CREATION 
  commons-args/src/main/java/org/apache/aurora/common/args/NoParser.java 
PRE-CREATION 
  commons-args/src/main/java/org/apache/aurora/common/args/Parser.java 
PRE-CREATION 
  commons-args/src/main/java/org/apache/aurora/common/args/ParserOracle.java 
PRE-CREATION 
  commons-args/src/main/java/org/apache/aurora/common/args/Positional.java 
PRE-CREATION 
  commons-args/src/main/java/org/apache/aurora/common/args/Verifier.java 
PRE-CREATION 
  commons-args/src/main/java/org/apache/aurora/common/args/VerifierFor.java 
PRE-CREATION 
  
commons-args/src/main/java/org/apache/aurora/common/args/apt/CmdLineProcessor.java
 PRE-CREATION 
  
commons-args/src/main/java/org/apache/aurora/common/args/apt/Configuration.java 
PRE-CREATION 
  
commons-args/src/main/resources/META-INF/services/javax.annotation.processing.Processor
 PRE-CREATION 
  commons/src/main/java/org/apache/aurora/common/application/Lifecycle.java 
PRE-CREATION 
  
commons/src/main/java/org/apache/aurora/common/application/ShutdownRegistry.java
 PRE-CREATION 
  commons/src/main/java/org/apache/aurora/common/application/ShutdownStage.java 
PRE-CREATION 
  commons/src/main/java/org/apache/aurora/common/args/ArgFilters.java 
PRE-CREATION 
  commons/src/main/java/org/apache/aurora/common/args/ArgScanner.java 
PRE-CREATION 
  commons/src/main/java/org/apache/aurora/common/args/Args.java PRE-CREATION 
  commons/src/main/java/org/apache/aurora/common/args/ArgumentInfo.java 
PRE-CREATION 
  commons/src/main/java/org/apache/aurora/common/args/OptionInfo.java 
PRE-CREATION 
  commons/src/main/java/org/apache/aurora/common/args/Parsers.java PRE-CREATION 
  commons/src/main/java/org/apache/aurora/common/args/PositionalInfo.java 
PRE-CREATION 
  

Re: Review Request 37818: Moved executor settings configuration to loadable JSON

[Renan DelValle]

> On Sept. 2, 2015, 4:45 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoader.java,
> >  line 87
> > 
> >
> > 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 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 

Re: Review Request 37818: Moved executor settings configuration to loadable JSON

[Renan DelValle]

> On Sept. 2, 2015, 4:45 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoader.java,
> >  line 87
> > 
> >
> > 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 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 

Re: Review Request 37818: Moved executor settings configuration to loadable JSON

[Bill Farner]

> On Sept. 2, 2015, 9:45 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoader.java,
> >  line 87
> > 
> >
> > 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 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 

Re: Review Request 37818: Moved executor settings configuration to loadable JSON

[Bill Farner]

> On Sept. 2, 2015, 9:45 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoader.java,
> >  line 87
> > 
> >
> > 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 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 

Re: Review Request 37818: Moved executor settings configuration to loadable JSON

[Renan DelValle]

> On Sept. 2, 2015, 4:45 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoader.java,
> >  line 87
> > 
> >
> > 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 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
> > 
> >
> > 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 

Re: Review Request 37818: Moved executor settings configuration to loadable JSON

[Bill Farner]

> On Sept. 2, 2015, 9:45 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoader.java,
> >  line 87
> > 
> >
> > 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 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.

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 

Re: Review Request 37818: Moved executor settings configuration to loadable JSON

[Bill Farner]

> On Sept. 2, 2015, 9:45 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoader.java,
> >  line 87
> > 
> >
> > 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 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 

Re: Review Request 37818: Moved executor settings configuration to loadable JSON

[Bill Farner]

> On Sept. 2, 2015, 9:45 a.m., Bill Farner wrote:
> > examples/vagrant/executors-config.json, lines 4-7
> > 
> >
> > 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?

Kevin - any opinion on this?


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

Re: Review Request 37818: Moved executor settings configuration to loadable JSON

[Renan DelValle]

> On Sept. 2, 2015, 4:45 p.m., Bill Farner wrote:
> > examples/vagrant/executors-config.json, lines 4-7
> > 
> >
> > 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.

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?


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

Re: Review Request 37818: Moved executor settings configuration to loadable JSON

[Bill Farner]

> On Aug. 31, 2015, 7:35 p.m., Bill Farner wrote:
> > examples/vagrant/executors-config.json, line 1
> > 
> >
> > Can you expand this example to include the command executor?  That's 
> > likely to be the first non-default executor folks will want to try.
> 
> Renan DelValle wrote:
> Sure thing. Although, be advised that the functionality for custom 
> executors is not included in this patch. This patch only lays the ground 
> work. The next patch will include the ability to actually select them and run 
> them.

Cool, that's actually what i wanted to see - a format change.  If we want the 
format to support multiple executors, we should use that format now so it works 
once multiple executors _are_ supported.


- Bill


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37818/#review97231
---


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

Re: Review Request 37818: Moved executor settings configuration to loadable JSON

[Bill Farner]

> On Sept. 2, 2015, 9:45 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoader.java,
> >  line 87
> > 
> >
> > 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

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.


- Bill


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37818/#review97468
---


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

Re: Review Request 37818: Moved executor settings configuration to loadable JSON

[Bill Farner]

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


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)


Can you omit this since it's blank?



examples/vagrant/executors-config.json (line 32)


Use single quotes to avoid escaping:

"'Hello World from Aurora!'"



src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 


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)


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)


remove



src/main/java/org/apache/aurora/scheduler/base/CommandUtil.java (lines 95 - 96)


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)


please add a javadoc for this class



src/main/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoader.java
 (line 62)


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)


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)


remove the ```\n```, same with the string below



src/main/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoader.java
 (line 87)


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 

Re: Review Request 37818: Moved executor settings configuration to loadable JSON

[Bill Farner]

> On Sept. 2, 2015, 9:45 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoader.java,
> >  line 87
> > 
> >
> > 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.

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


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37818/#review97468
---


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

Re: Review Request 37818: Moved executor settings configuration to loadable JSON

[Renan DelValle]

> On Sept. 2, 2015, 4:45 p.m., Bill Farner wrote:
> > examples/vagrant/executors-config.json, lines 17-18
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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 

Re: Review Request 37818: Moved executor settings configuration to loadable JSON

[Kevin Sweeney]

> On Sept. 2, 2015, 9:45 a.m., Bill Farner wrote:
> > examples/vagrant/executors-config.json, lines 4-7
> > 
> >
> > 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.

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


- Kevin


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37818/#review97468
---


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

Re: Review Request 37818: Moved executor settings configuration to loadable JSON

[Bill Farner]

> On Sept. 2, 2015, 9:45 a.m., Bill Farner wrote:
> > examples/vagrant/executors-config.json, lines 4-7
> > 
> >
> > 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

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.


- Bill


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37818/#review97468
---


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

Re: Review Request 37818: Moved executor settings configuration to loadable JSON

[Renan DelValle]

> On Sept. 1, 2015, 2:35 a.m., Bill Farner wrote:
> > examples/vagrant/executors-config.json, line 1
> > 
> >
> > Can you expand this example to include the command executor?  That's 
> > likely to be the first non-default executor folks will want to try.

Sure thing. Although, be advised that the functionality for custom executors is 
not included in this patch. This patch only lays the ground work. The next 
patch will include the ability to actually select them and run them.


- Renan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37818/#review97231
---


On Sept. 1, 2015, 12:37 a.m., Renan DelValle wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37818/
> ---
> 
> (Updated Sept. 1, 2015, 12:37 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 
> 0c440b5cd5b939872c1ee05d048bf739bfa977cb 
>   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/no-value-URI.json
>  PRE-CREATION 
>   
> src/test/resources/org/apache/aurora/scheduler/configuration/thermos-settings-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
> 
>

Re: Review Request 37818: Moved executor settings configuration to loadable JSON

[Renan DelValle]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37818/
---

(Updated Sept. 1, 2015, 12:37 a.m.)


Review request for Aurora.


Changes
---

New schema implemented with support for URIs as outlined by the Mesos spec. 
Separation of command to be executed from resources to be fetched in 
preparation for support for cusotm executors.

Deserializers changed to support URI objects in schema.
Empty or non-existent globalContainerMounts is represented as an empty list in 
ExecutorSettings.

@ReviewBot retry


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 (updated)
-

  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 
0c440b5cd5b939872c1ee05d048bf739bfa977cb 
  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/no-value-URI.json 
PRE-CREATION 
  
src/test/resources/org/apache/aurora/scheduler/configuration/thermos-settings-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

Re: Review Request 37818: Moved executor settings configuration to loadable JSON

[Aurora ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37818/#review97220
---

Ship it!


Master (c8e65d3) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Sept. 1, 2015, 12:37 a.m., Renan DelValle wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37818/
> ---
> 
> (Updated Sept. 1, 2015, 12:37 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 
> 0c440b5cd5b939872c1ee05d048bf739bfa977cb 
>   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/no-value-URI.json
>  PRE-CREATION 
>   
> src/test/resources/org/apache/aurora/scheduler/configuration/thermos-settings-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
> 
>

Re: Review Request 37818: Moved executor settings configuration to loadable JSON

[Kevin Sweeney]

 On Aug. 26, 2015, 3:27 p.m., Kevin Sweeney wrote:
  examples/vagrant/executors-config-new.json, line 18
  https://reviews.apache.org/r/37818/diff/1/?file=1055421#file1055421line18
 
  this isn't a global property - can it be pushed into a custom 
  configuration object?
 
 Renan DelValle wrote:
 We can, but this change causes a ripple effect. If we push this into a 
 custom schema, we have to provide a way of unpacking custom configuration 
 object into useful mesos data fields or information usefol to the executor. 
 We need to come to a decision on the best way of doing this if we go for it. 
 Tangentially, as far as I can tell this is only used when building container 
 volumes.

As this is greenfield code I'm okay with a bit of a ripple - I'd much rather 
cluster operators have to write this config file once than change it as our 
design for it evolves.


 On Aug. 26, 2015, 3:27 p.m., Kevin Sweeney wrote:
  examples/vagrant/executors-config-new.json, line 19
  https://reviews.apache.org/r/37818/diff/1/?file=1055421#file1055421line19
 
  It would be ergnomically nice to default this to an empty list if it's 
  left unset.
 
 Renan DelValle wrote:
 I'll see if there's a way of doing that. If you come across anything, let 
 me know.

A bit of Googling suggests that setting defaults in a default constructor works 
with GSON here. In fact it seems that just about any way you can think of doing 
it GSON supports defaults (confirmed myself here: 
https://gist.github.com/kevints/d1e26514468863e8c088).

Setting defaults in an initializer works too - I suggest following the pattern 
in `ImmutableDefaultTest`, or `ImmutableInitializeOverrideTest` if you have a 
`@VisibleForTesting` constructor.


- Kevin


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37818/#review96605
---


On Aug. 26, 2015, 3:19 p.m., Renan DelValle wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/37818/
 ---
 
 (Updated Aug. 26, 2015, 3:19 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-new.json PRE-CREATION 
   examples/vagrant/executors-config.json PRE-CREATION 
   examples/vagrant/upstart/aurora-scheduler-kerberos.conf 
 744b4a35c61e749734e222b3d4cbd296927665aa 
   examples/vagrant/upstart/aurora-scheduler.conf 
 789a3a0315e8530880999432aa9b1e7d0f57d1ff 
   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
 0c440b5cd5b939872c1ee05d048bf739bfa977cb 
   
 src/main/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoader.java
  PRE-CREATION 
   src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java 
 b3c913892248e4a9a8111412307463985f5ca97f 
   
 src/test/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoaderTest.java
  PRE-CREATION 
   
 src/test/resources/org/apache/aurora/scheduler/configuration/executor-settings-thermos-no-observer.json
  PRE-CREATION 
   
 src/test/resources/org/apache/aurora/scheduler/configuration/thermos-settings-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
 

Re: Review Request 37818: Moved executor settings configuration to loadable JSON

[Aurora ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37818/#review96733
---

Ship it!


Master (06ddaad) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing @ReviewBot 
retry

- Aurora ReviewBot


On Aug. 26, 2015, 10:19 p.m., Renan DelValle wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/37818/
 ---
 
 (Updated Aug. 26, 2015, 10:19 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-new.json PRE-CREATION 
   examples/vagrant/executors-config.json PRE-CREATION 
   examples/vagrant/upstart/aurora-scheduler-kerberos.conf 
 744b4a35c61e749734e222b3d4cbd296927665aa 
   examples/vagrant/upstart/aurora-scheduler.conf 
 789a3a0315e8530880999432aa9b1e7d0f57d1ff 
   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
 0c440b5cd5b939872c1ee05d048bf739bfa977cb 
   
 src/main/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoader.java
  PRE-CREATION 
   src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java 
 b3c913892248e4a9a8111412307463985f5ca97f 
   
 src/test/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoaderTest.java
  PRE-CREATION 
   
 src/test/resources/org/apache/aurora/scheduler/configuration/executor-settings-thermos-no-observer.json
  PRE-CREATION 
   
 src/test/resources/org/apache/aurora/scheduler/configuration/thermos-settings-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
 

Review Request 37818: Moved executor settings configuration to loadable JSON

[Renan DelValle]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37818/
---

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-new.json PRE-CREATION 
  examples/vagrant/executors-config.json PRE-CREATION 
  examples/vagrant/upstart/aurora-scheduler-kerberos.conf 
744b4a35c61e749734e222b3d4cbd296927665aa 
  examples/vagrant/upstart/aurora-scheduler.conf 
789a3a0315e8530880999432aa9b1e7d0f57d1ff 
  src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
0c440b5cd5b939872c1ee05d048bf739bfa977cb 
  
src/main/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoader.java
 PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java 
b3c913892248e4a9a8111412307463985f5ca97f 
  
src/test/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoaderTest.java
 PRE-CREATION 
  
src/test/resources/org/apache/aurora/scheduler/configuration/executor-settings-thermos-no-observer.json
 PRE-CREATION 
  
src/test/resources/org/apache/aurora/scheduler/configuration/thermos-settings-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

Re: Review Request 37818: Moved executor settings configuration to loadable JSON

[Kevin Sweeney]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37818/#review96605
---


Quick (incomplete) review on the design of the config object


examples/vagrant/executors-config-new.json (lines 13 - 15)
https://reviews.apache.org/r/37818/#comment152193

do these have defaults that we can omit? also I don't think we want to 
cache in the vagrant environment as we want changes to be reflected in new 
tasks immediately when developing.



examples/vagrant/executors-config-new.json (line 14)
https://reviews.apache.org/r/37818/#comment152195

don't extract this



examples/vagrant/executors-config-new.json (line 18)
https://reviews.apache.org/r/37818/#comment152194

this isn't a global property - can it be pushed into a custom configuration 
object?



examples/vagrant/executors-config-new.json (line 19)
https://reviews.apache.org/r/37818/#comment152192

It would be ergnomically nice to default this to an empty list if it's left 
unset.



examples/vagrant/executors-config.json (line 1)
https://reviews.apache.org/r/37818/#comment152191

delete this file? It looks like it has been superceded by the new format 
below



src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java (line 114)
https://reviews.apache.org/r/37818/#comment152196

drop redundant use of `this`, here and below.


- Kevin Sweeney


On Aug. 26, 2015, 3:19 p.m., Renan DelValle wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/37818/
 ---
 
 (Updated Aug. 26, 2015, 3:19 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-new.json PRE-CREATION 
   examples/vagrant/executors-config.json PRE-CREATION 
   examples/vagrant/upstart/aurora-scheduler-kerberos.conf 
 744b4a35c61e749734e222b3d4cbd296927665aa 
   examples/vagrant/upstart/aurora-scheduler.conf 
 789a3a0315e8530880999432aa9b1e7d0f57d1ff 
   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
 0c440b5cd5b939872c1ee05d048bf739bfa977cb 
   
 src/main/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoader.java
  PRE-CREATION 
   src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java 
 b3c913892248e4a9a8111412307463985f5ca97f 
   
 src/test/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoaderTest.java
  PRE-CREATION 
   
 src/test/resources/org/apache/aurora/scheduler/configuration/executor-settings-thermos-no-observer.json
  PRE-CREATION 
   
 src/test/resources/org/apache/aurora/scheduler/configuration/thermos-settings-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
 

Re: Review Request 37818: Moved executor settings configuration to loadable JSON

[Renan DelValle]

 On Aug. 26, 2015, 10:27 p.m., Kevin Sweeney wrote:
  src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java, line 
  114
  https://reviews.apache.org/r/37818/diff/1/?file=1055427#file1055427line114
 
  drop redundant use of `this`, here and below.

Done, apologies, this one slipped through the cracks from the previous reviews.


 On Aug. 26, 2015, 10:27 p.m., Kevin Sweeney wrote:
  examples/vagrant/executors-config.json, line 1
  https://reviews.apache.org/r/37818/diff/1/?file=1055422#file1055422line1
 
  delete this file? It looks like it has been superceded by the new 
  format below

This first cut wasn't supposed to include the new schema. I was gonna implement 
it in the second patch but I guess now is as good a time as any to get this in.


 On Aug. 26, 2015, 10:27 p.m., Kevin Sweeney wrote:
  examples/vagrant/executors-config-new.json, line 19
  https://reviews.apache.org/r/37818/diff/1/?file=1055421#file1055421line19
 
  It would be ergnomically nice to default this to an empty list if it's 
  left unset.

I'll see if there's a way of doing that. If you come across anything, let me 
know.


 On Aug. 26, 2015, 10:27 p.m., Kevin Sweeney wrote:
  examples/vagrant/executors-config-new.json, line 18
  https://reviews.apache.org/r/37818/diff/1/?file=1055421#file1055421line18
 
  this isn't a global property - can it be pushed into a custom 
  configuration object?

We can, but this change causes a ripple effect. If we push this into a custom 
schema, we have to provide a way of unpacking custom configuration object into 
useful mesos data fields or information usefol to the executor. We need to come 
to a decision on the best way of doing this if we go for it. Tangentially, as 
far as I can tell this is only used when building container volumes.


 On Aug. 26, 2015, 10:27 p.m., Kevin Sweeney wrote:
  examples/vagrant/executors-config-new.json, lines 13-15
  https://reviews.apache.org/r/37818/diff/1/?file=1055421#file1055421line13
 
  do these have defaults that we can omit? also I don't think we want to 
  cache in the vagrant environment as we want changes to be reflected in new 
  tasks immediately when developing.

As per the mesos documentation, all options but the string value are optional. 
What isn't very clear is how the extract is true by default if it is optional. 
(source: https://github.com/apache/mesos/blob/master/docs/fetcher.md)


- Renan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37818/#review96605
---


On Aug. 26, 2015, 10:19 p.m., Renan DelValle wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/37818/
 ---
 
 (Updated Aug. 26, 2015, 10:19 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-new.json PRE-CREATION 
   examples/vagrant/executors-config.json PRE-CREATION 
   examples/vagrant/upstart/aurora-scheduler-kerberos.conf 
 744b4a35c61e749734e222b3d4cbd296927665aa 
   examples/vagrant/upstart/aurora-scheduler.conf 
 789a3a0315e8530880999432aa9b1e7d0f57d1ff 
   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
 0c440b5cd5b939872c1ee05d048bf739bfa977cb 
   
 src/main/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoader.java
  PRE-CREATION 
   src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java 
 b3c913892248e4a9a8111412307463985f5ca97f 
   
 src/test/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoaderTest.java
  PRE-CREATION 
   
 src/test/resources/org/apache/aurora/scheduler/configuration/executor-settings-thermos-no-observer.json
  PRE-CREATION 
   
 src/test/resources/org/apache/aurora/scheduler/configuration/thermos-settings-example.json
  PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/37818/diff/
 
 
 Testing
 ---