> On Sept. 2, 2015, 9:45 a.m., Bill Farner wrote:
> > examples/vagrant/executors-config.json, lines 4-7
> > <https://reviews.apache.org/r/37818/diff/3/?file=1061919#file1061919line4>
> >
> >     The code later converts this array into a single command string.  I 
> > suggest we just make this a string here (and save some code down the line).
> 
> Renan DelValle wrote:
>     We can go ahead and do that. Kevin thought it should be the array way, 
> but I'm not sure how strongly he feels about it. The extra code isn't too bad 
> because we can use String.join() form Java 8. Either way, I'm fine with 
> either. The single string may eliminate any suspicions that we're breaking 
> something in a command when joining the array.
> 
> Kevin Sweeney wrote:
>     Bill, I prefer the array mode as it's more explicit for dealing with 
> magic characters in shell commands (like " (){}$").
>     
>     You can still explicitly do
>     
>     ```
>     ["sh", "-c", "PATH=/opt/aurora/executor/bin:$PATH thermos"]
>     ```
>     
>     When you prefer the old form, but this form will not automatically expand 
> environment variables or do other shell magic for you.
>     
>     Renan - can you update CommandUtil to use the `arguments`[1] field 
> instead of `shell` and `value` (or drop a TODO for a later patch)? Changing 
> back to `String.join` defeats part of the purpose of the array form here.
>     
>     [1] 
> https://github.com/apache/mesos/blob/master/include/mesos/mesos.proto#L408-L423

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 
>   
> src/test/resources/org/apache/aurora/scheduler/configuration/executor-settings-thermos-no-observer.json
>  PRE-CREATION 
>   
> src/test/resources/org/apache/aurora/scheduler/configuration/multiple-executor-example.json
>  PRE-CREATION 
>   
> src/test/resources/org/apache/aurora/scheduler/configuration/no-value-URI.json
>  PRE-CREATION 
>   
> src/test/resources/org/apache/aurora/scheduler/configuration/single-executor-example.json
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37818/diff/
> 
> 
> Testing
> -------
> 
> ./build-support/jenkins/build.sh: directory sandbox failed but it may be a 
> flaky test
> bash src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Renan DelValle
> 
>

Reply via email to