aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Renan DelValle" <rdelv...@binghamton.edu>
Subject Re: Review Request 37818: Moved executor settings configuration to loadable JSON
Date Thu, 03 Sep 2015 22:33:24 GMT


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

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

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message