> On July 15, 2015, 7:08 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/app/ExecutorSettingsLoader.java, > > line 57 > > <https://reviews.apache.org/r/36289/diff/2/?file=1011919#file1011919line57> > > > > Please model your configuration as an object, and let Gson do the heavy > > lifting for you. As a starting point, it will probably be something like > > this (source trimmed for conciseness): > > > > > > ``` > > class ExecutorConfig { > > String name; > > String path; > > String arguments; > > Set<String> resources; > > ... > > } > > ``` > > > > Then have gson read `List<ExecutorConfig>`. > > > > We'll probably want to have one field that is something like > > `JsonObject` so that the executor can have a snippet of custom schema. > > Renan DelValle wrote: > Aw shucks! Seems so obvious now that you mention it. I went ahead and did > that, made my life a lot easier. > > One thing to note is that thrift already generates an ExecutorConfig > class at org/apache/aurora/gen/ExecutorConfig.java > > Is it OK to have a second class in > org/apache/aurora/scheduler/app/ExecutorConfig.java with the same name? > > Bill Farner wrote: > :-) > > I'd slightly perfer a separate name over fully-qualifying the name here - > so yeah, feel free to change it to whatever you see fit and we can iterate if > necessary. > > Renan DelValle wrote: > Bill, could you expand on this idea of having a custom schema as a > JsonObject? Could you provide me with an example? > > Bill Farner wrote: > Oh, all i mean is for common code to handle the required schema elements. > Drawing from the POJO in my original comment, you could hav: > > ``` > class ExecutorConfig { > String name; > String path; > String arguments; > Set<String> resources; > JsonObject instructions; > } > ``` > > Gson will parse this, only requiring that the `instructions` field be a > valid JSON object. For the purposes of _that_ parsing code, it is arbitrary > JSON. You will want to create an interface for executors. Not having > thought much about the types, it could be as simple as: > > ``` > interface TaskBuilder { > TaskInfo buildFrom(ExecutorConfig config); > } > ``` > > As mentioned before, you would have multiple of these, keyed by the > executor name: > > ``` > Map<String, TaskBuilder> executorTypes; > ``` > > This provides sufficient abstraction for the thermos `TaskBuilder` to > parse its more complex process graph from `ExecutorConfig.instructions`, > while the builder invoking the command executor can use a much simpler schema: > > ``` > { > "name": "command", > "instructions": { > "command": "echo hello" > } > } > ``` > > Meghdoot Bhattacharya wrote: > Bill can you explain the motivation of this direction. Why would we want > scheduler to handle these? What is custom about building a Mesos task other > than the well know command vs other executor taskbuilder differences. The > client would pass the "custom data" for executor in the "data" field of the > executorconfig object in thrift and that maps to the mesos field. Only for > command executor we can make the assumption that "data" has the command > string. Server side configuration is pretty static. If at some point we > expose this entirely in client side to set then everything can become dynamic. > In the example you provided above, there is no static instructions for > command executor, because command string will come in as data. I feel special > instructions to executor can be encoded in the data blob that executor can > find out after unmarshalling it. How will Mesos Task creation vary per custom > executor because the fields are pretty generic. > > So, for thermos, what fields instructions object going to have. > thermosobserver, executoroverhead etc?. These are not fields required for > setting in Mesos task. In the end, if we need to introduce this field, this > should be strictly optional field and null should be a valid option. We have > examples of executors where we would not have any usage of this instructions > field. Other than just making config changes on the scheduler side, I dont > have to be forced to add plugins or code in the scheduler itself. > > Let us know
Yes, my mistake - the `instructions` field may be unnecssary right now. The intent is that if/when there are params that are specific to an executor, they should go into a generic field whose schema is defined by each executor launcher. - Bill ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36289/#review91793 ----------------------------------------------------------- On Aug. 18, 2015, 6:10 a.m., Renan DelValle wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36289/ > ----------------------------------------------------------- > > (Updated Aug. 18, 2015, 6:10 a.m.) > > > Review request for Aurora and Bill Farner. > > > Repository: aurora > > > Description > ------- > > What was done: > ============== > Added support for dynamically chosing an executor that's definied in a server > side config file. > Removed command line arguments that were moved over to the config file. > Updated existing code to reflect the use of a Map instead of a single > ExecutorSettings object. > > Future: > ======= > Create an offshoot of the current client that allows to send thrift calls > with different executor configs which will allow use of custom executors. > Some work on this has already been done and will be published ASAP for > testing. > > > Diffs > ----- > > api/src/main/thrift/org/apache/aurora/gen/api.thrift > f792be0ad393072b4a4ec525363e06cfd16b63d0 > 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/ResourceSlot.java > e5953bbf02fc2b08fbdff5c25b5389c5a209dfca > src/main/java/org/apache/aurora/scheduler/app/ExecutorConfiguration.java > PRE-CREATION > src/main/java/org/apache/aurora/scheduler/app/ExecutorSettingsLoader.java > PRE-CREATION > src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java > e74b36bc5f85e5ae5fbb2e0b1e34961251739d9e > src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java > 10389640c87b203386313ab79204ea936272d350 > src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java > b3c913892248e4a9a8111412307463985f5ca97f > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java > ff6eb980292c05e35dcf68104c870a7bef95629a > src/main/java/org/apache/aurora/scheduler/preemptor/PreemptionVictim.java > 8162323816aedc711a3af84cd499250b78718ab3 > > src/main/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilter.java > a0e71e1c74f67b8836e7da5418012f342977f661 > src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java > 50e7fc91108993e547869df5b9e5c925fb89a225 > > src/test/java/org/apache/aurora/scheduler/app/ExecutorConfigurationTest.java > PRE-CREATION > > src/test/java/org/apache/aurora/scheduler/app/ExecutorSettingsLoaderTest.java > PRE-CREATION > src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java > 37772d0b75d022f072af10d82d096981680e193f > > src/test/java/org/apache/aurora/scheduler/events/NotifyingSchedulingFilterTest.java > 608af1afe6fc27c8c597490e88fed75580076c95 > > src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java > b2327a47374d81b59886c1e4575ded8340322db7 > > src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java > 02fe96445148d1e14d85dc7a6fa386d84a8a8c70 > src/test/java/org/apache/aurora/scheduler/mesos/TaskExecutors.java > 6a80503aeb2058e8f8065285adc151197d2d14d6 > > src/test/java/org/apache/aurora/scheduler/preemptor/ClusterStateImplTest.java > a1ac922d471013779710e02c0c9ca9f84b506807 > > src/test/java/org/apache/aurora/scheduler/preemptor/PendingTaskProcessorTest.java > b9cb5bfe9f89a8bfdb96b6eeb1998ed105963484 > > src/test/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilterTest.java > 66f20c6a63b331353c467cde5521f21e4df49e2d > > src/test/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimTest.java > 09380f95a7d9405f770513db35d2a45d23d89b61 > src/test/java/org/apache/aurora/scheduler/preemptor/PreemptorImplTest.java > b07ff7babd217dac4153831a0d78325bcb72b306 > > src/test/resources/org/apache/aurora/scheduler/app/executor-settings-example.json > PRE-CREATION > > src/test/resources/org/apache/aurora/scheduler/app/executor-settings-mesos-command-example.json > PRE-CREATION > > src/test/resources/org/apache/aurora/scheduler/app/executor-settings-thermos-no-observer.json > PRE-CREATION > > Diff: https://reviews.apache.org/r/36289/diff/ > > > Testing > ------- > > Ran jenkins build test, passed all tests, code style checks, findbugs check, > and PMD. > Ran end to end, failed upon reaching kerberos tests. > > > Thanks, > > Renan DelValle > >
