> 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" > } > } > ```
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 - Meghdoot ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36289/#review91793 ----------------------------------------------------------- On July 22, 2015, 7:28 p.m., Renan DelValle wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36289/ > ----------------------------------------------------------- > > (Updated July 22, 2015, 7:28 p.m.) > > > Review request for Aurora and Bill Farner. > > > Repository: aurora > > > Description > ------- > > What was done: > ============== > Added support for custom executors in the Scheduler via a config file. > Removed command line arguments that were moved over to the config file. > > Future: > ======= > Extending the client to support custom executors and the mesos-executor. > > Caveats: > ======== > This contains initial config file with support for thermos and limited > support for the mesos commandline executor. Mesos-command line executor needs > support from the client side in order to function at a better capacity. > > Currently, this uses the current client to launch both tasks, meaning as long > as the client sends a thrift call, the scheduler will schedule a task, be it > a mesos-command task with a preconfigured command temporarily set in the > config file or a custom executor task. > > *Support for custom executors in the client must be added in order to fully > utilize this feature.* > > > Diffs > ----- > > examples/vagrant/upstart/aurora-scheduler-kerberos.conf > 85052ac8e4dde85fbcd85ce839d0647f5632d74b > examples/vagrant/upstart/aurora-scheduler.conf > f261c8dcc760151d5a41a986d867585c3a544123 > src/dist/etc/executors.json PRE-CREATION > src/main/java/org/apache/aurora/scheduler/app/ExecutorSettingsLoader.java > PRE-CREATION > src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java > 554a380bdb4ef69561259cdbfbc361694041571e > src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java > 325f55640648151ae19e0c18c6961aeff10bfac3 > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java > c0d165ad34e46653dad95918e0058ebd3f2ee57f > > src/test/java/org/apache/aurora/scheduler/app/ExecutorSettingsLoaderTest.java > PRE-CREATION > src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java > 23c2693f1dfd589043c60ab22e302fb81e62335d > > src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java > c0cadfb34ade55bdb38ab2c0f89499bd6e8fa97a > src/test/java/org/apache/aurora/scheduler/mesos/TaskExecutors.java > ebd81775c5c9f0ef5c309869df1d12dca3ddbdd7 > > src/test/resources/org/apache/aurora/scheduler/app/executor-settings-example.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. > > > Thanks, > > Renan DelValle > >
