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