Re: Review Request 36289: Custom executor support for Scheduler
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36289/#review107128 --- This has been idle a good while and it appears wfarner has picked up work on https://issues.apache.org/jira/browse/AURORA-1376, is it safe to discard? - John Sirois On Aug. 18, 2015, 12: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, 12: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,
Re: Review Request 36289: Custom executor support for Scheduler
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36289/#review95873
---
Partial review - stopped at the question of how to ensure the precondition
check added in SchedulingFilterImpl always passes. I think this patch needs to
add startup validation and a backfill strategy.
api/src/main/thrift/org/apache/aurora/gen/api.thrift (line 46)
https://reviews.apache.org/r/36289/#comment151027
Hmm - why does this magic string exist? Any idea what the implications of
changing it are?
examples/vagrant/executors-config.json (lines 5 - 6)
https://reviews.apache.org/r/36289/#comment151028
Is there a difference between path and resources as far as Mesos is
concerned?
src/main/java/org/apache/aurora/scheduler/app/ExecutorConfiguration.java (line
34)
https://reviews.apache.org/r/36289/#comment151037
Given that you use this type in a `Set` you will need to define `hashCode`
and `equals` here (and for all contained types).
src/main/java/org/apache/aurora/scheduler/app/ExecutorConfiguration.java (line
37)
https://reviews.apache.org/r/36289/#comment151034
Make these `final` and add a constructor for testing?
src/main/java/org/apache/aurora/scheduler/app/ExecutorConfiguration.java (line
48)
https://reviews.apache.org/r/36289/#comment151038
Needs `hashCode` and `equals`
src/main/java/org/apache/aurora/scheduler/app/ExecutorConfiguration.java (line
85)
https://reviews.apache.org/r/36289/#comment151031
remove redundant use of `this`, here and below
src/main/java/org/apache/aurora/scheduler/app/ExecutorSettingsLoader.java
(lines 40 - 44)
https://reviews.apache.org/r/36289/#comment151036
No JavaDoc for private methods - use a comment instead:
```java
private ExecutorSettingsLoader() {
// Utility class.
}
```
src/main/java/org/apache/aurora/scheduler/app/ExecutorSettingsLoader.java (line
60)
https://reviews.apache.org/r/36289/#comment151035
Take a `File` here instead of a `String`.
Return a narrower type, like `SetExecutorSettings` (but probably
`ListExecutorSetttings`, see below.
src/main/java/org/apache/aurora/scheduler/app/ExecutorSettingsLoader.java (line
63)
https://reviews.apache.org/r/36289/#comment151039
Prefer `ImmutableSet.Builder`
src/main/java/org/apache/aurora/scheduler/app/ExecutorSettingsLoader.java (line
66)
https://reviews.apache.org/r/36289/#comment151040
You leak a file descriptor here - consider just using
`Files.asCharSource(configFile, StandardCharsets.UTF_8)` and opening it in a
try-with-resources block:
```java
CharSource configFileSource = Files.asCharSource(configFile,
StandardCharsets.UTF_8);
try (Reader reader = configFileSource.openBufferedReader()) {
gson.read(...);
}
```
src/main/java/org/apache/aurora/scheduler/app/ExecutorSettingsLoader.java (line
68)
https://reviews.apache.org/r/36289/#comment151047
nit: no space between `}` and `.`
src/main/java/org/apache/aurora/scheduler/app/ExecutorSettingsLoader.java (line
75)
https://reviews.apache.org/r/36289/#comment151046
Mentioning the filename would make this easier to debug.
src/main/java/org/apache/aurora/scheduler/app/ExecutorSettingsLoader.java
(lines 76 - 77)
https://reviews.apache.org/r/36289/#comment151044
This is actually caused by the platform JVM not having support for an
encoding (which is impossible for UTF8 as it's required by the standard). Use
the `asCharSource` suggestion with the constant from `StandardCharsets` to
avoid needing to handle this.
src/main/java/org/apache/aurora/scheduler/app/ExecutorSettingsLoader.java (line
79)
https://reviews.apache.org/r/36289/#comment151045
You'll want to concat the error message here as well as the second param
will just print the stack trace. Also a bit more context would be useful. e.g.
```java
throw new ExecutorSettingsException(Error parsing executor settings JSON:
+ e, e);
```
src/main/java/org/apache/aurora/scheduler/app/ExecutorSettingsLoader.java (line
81)
https://reviews.apache.org/r/36289/#comment151041
Can you determine which parameter is missing? This would be a very
frustrating error message to debug.
src/main/java/org/apache/aurora/scheduler/app/ExecutorSettingsLoader.java (line
84)
https://reviews.apache.org/r/36289/#comment151048
You read this as a `List`, any reason not to use `ImmutableList.copyOf`
here instead?
src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java (line 81)
https://reviews.apache.org/r/36289/#comment151049
`ArgFile` and add a `@CanRead` annotation for more useful validation.
src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java (line 170)
https://reviews.apache.org/r/36289/#comment151050
This needs to re-throw, otherwise initialization
Re: Review Request 36289: Custom executor support for Scheduler
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36289/#review95689 --- Master (22f9cbb) is red with this patch. ./build-support/jenkins/build.sh src.test.python.apache.aurora.client.cli.plugins . SUCCESS src.test.python.apache.aurora.client.cli.quota . SUCCESS src.test.python.apache.aurora.client.cli.sla . SUCCESS src.test.python.apache.aurora.client.cli.supdate . SUCCESS src.test.python.apache.aurora.client.cli.task . SUCCESS src.test.python.apache.aurora.client.cli.update . SUCCESS src.test.python.apache.aurora.client.cli.version . SUCCESS src.test.python.apache.aurora.client.config . SUCCESS src.test.python.apache.aurora.client.hooks.hooked_api . SUCCESS src.test.python.apache.aurora.client.hooks.non_hooked_api . SUCCESS src.test.python.apache.aurora.common.test_aurora_job_key . SUCCESS src.test.python.apache.aurora.common.test_cluster . SUCCESS src.test.python.apache.aurora.common.test_cluster_option . SUCCESS src.test.python.apache.aurora.common.test_clusters . SUCCESS src.test.python.apache.aurora.common.test_http_signaler . SUCCESS src.test.python.apache.aurora.common.test_pex_version . SUCCESS src.test.python.apache.aurora.common.test_shellify . SUCCESS src.test.python.apache.aurora.common.test_transport . SUCCESS src.test.python.apache.aurora.config.test_base . SUCCESS src.test.python.apache.aurora.config.test_constraint_parsing . SUCCESS src.test.python.apache.aurora.config.test_loader . SUCCESS src.test.python.apache.aurora.config.test_thrift . SUCCESS src.test.python.apache.aurora.executor.common.path_detector . SUCCESS src.test.python.apache.aurora.executor.common.task_info . SUCCESS src.test.python.apache.aurora.executor.executor_base . SUCCESS src.test.python.apache.aurora.executor.executor_vars . SUCCESS src.test.python.apache.aurora.executor.http_lifecycle . SUCCESS src.test.python.apache.aurora.executor.status_manager . SUCCESS src.test.python.apache.aurora.executor.thermos_task_runner . FAILURE src.test.python.apache.thermos.cli.commands.commands . SUCCESS src.test.python.apache.thermos.cli.common . SUCCESS src.test.python.apache.thermos.cli.main . SUCCESS src.test.python.apache.thermos.common.test_pathspec . SUCCESS src.test.python.apache.thermos.core.test_runner_integration . SUCCESS src.test.python.apache.thermos.monitoring.test_disk . SUCCESS FAILURE [31m FAILURE[0m I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot 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
Re: Review Request 36289: Custom executor support for Scheduler
On Aug. 18, 2015, 3:06 a.m., Aurora ReviewBot wrote: Master (22f9cbb) is red with this patch. ./build-support/jenkins/build.sh :api:checkPython :api:generateThriftEntitiesJava :api:classesThriftEntities :api:compileJava UP-TO-DATE :api:generateThriftResources :api:processResources UP-TO-DATE :api:classes :api:jar :compileJavaNote: Writing file:/home/jenkins/jenkins-slave/workspace/AuroraBot/dist/classes/main/com/twitter/common/args/apt/cmdline.arg.info.txt.2 :processResources :classes :jar :startScripts :distTar :distZip :assemble :compileJmhJavaNote: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java uses or overrides a deprecated API. Note: Recompile with -Xlint:deprecation for details. :processJmhResources UP-TO-DATE :jmhClasses :checkstyleJmh :jsHint :checkstyleMain[ant:checkstyle] /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/app/ExecutorConfiguration.java:23:8: Unused import - com.google.gson.JsonObject. FAILED FAILURE: Build failed with an exception. * What went wrong: Execution failed for task ':checkstyleMain'. Checkstyle rule violations were found. See the report at: file:///home/jenkins/jenkins-slave/workspace/AuroraBot/dist/reports/checkstyle/main.xml * Try: Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. BUILD FAILED Total time: 1 mins 35.518 secs I will refresh this build result if you post a review containing @ReviewBot retry @ReviewBot retry - Renan --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36289/#review95680 --- 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
Re: Review Request 36289: Custom executor support for Scheduler
--- 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. Changes --- Commited unused import that I had forgotten to :B 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 (updated) - 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
Re: Review Request 36289: Custom executor support for Scheduler
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36289/ --- (Updated Aug. 18, 2015, 12:28 a.m.) Review request for Aurora and Bill Farner. Changes --- Added support for dynamically chosing an executor via the name provided in the thrift call using MapBinder. Mesos-command executor now pulls command from data field inside of executor settings in the thrift call. Removed executor name as an argument. Changed code to allow GSON to do the heavy lifting. Updated test classes to reflect the use of a Map instead of a single instance of ExecutorSettings. Tested using different executors on one scheduler simultaneously using a modified version of the client. 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 (updated) - 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:
Re: Review Request 36289: Custom executor support for Scheduler
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36289/#review95680 --- Master (22f9cbb) is red with this patch. ./build-support/jenkins/build.sh :api:checkPython :api:generateThriftEntitiesJava :api:classesThriftEntities :api:compileJava UP-TO-DATE :api:generateThriftResources :api:processResources UP-TO-DATE :api:classes :api:jar :compileJavaNote: Writing file:/home/jenkins/jenkins-slave/workspace/AuroraBot/dist/classes/main/com/twitter/common/args/apt/cmdline.arg.info.txt.2 :processResources :classes :jar :startScripts :distTar :distZip :assemble :compileJmhJavaNote: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java uses or overrides a deprecated API. Note: Recompile with -Xlint:deprecation for details. :processJmhResources UP-TO-DATE :jmhClasses :checkstyleJmh :jsHint :checkstyleMain[ant:checkstyle] /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/app/ExecutorConfiguration.java:23:8: Unused import - com.google.gson.JsonObject. FAILED FAILURE: Build failed with an exception. * What went wrong: Execution failed for task ':checkstyleMain'. Checkstyle rule violations were found. See the report at: file:///home/jenkins/jenkins-slave/workspace/AuroraBot/dist/reports/checkstyle/main.xml * Try: Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. BUILD FAILED Total time: 1 mins 35.518 secs I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On Aug. 18, 2015, 12:33 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, 12:33 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
Re: Review Request 36289: Custom executor support for Scheduler
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36289/ --- (Updated Aug. 18, 2015, 12:33 a.m.) Review request for Aurora and Bill Farner. Repository: aurora Description (updated) --- 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 (updated) --- 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
Re: Review Request 36289: Custom executor support for Scheduler
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;
SetString resources;
...
}
```
Then have gson read `ListExecutorConfig`.
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?
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;
SetString 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:
```
MapString, 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
---
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
Re: Review Request 36289: Custom executor support for Scheduler
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;
SetString resources;
...
}
```
Then have gson read `ListExecutorConfig`.
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.
Bill, could you expand on this idea of having a custom schema as a JsonObject?
Could you provide me with an example?
- Renan
---
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
Re: Review Request 36289: Custom executor support for Scheduler
On July 15, 2015, 12:08 p.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java, lines 87-115 https://reviews.apache.org/r/36289/diff/2/?file=1011920#file1011920line87 In this change, we cannot remove these arguments as it breaks compatibility. In this change, we'll need to synthesize an entry as though it were read from the json file, and log a big warning about the deprecation. Can you elaborate on the compatibility breakage? IMO changing command-line args across releases doesn't rise to the level of a compatibility break, as long as the change is isolated to a single component. This doesn't change the way the scheduler behaves on the network from the perspective of other components. The deprecation warning could just as easily be an error, pointing to documentation for the new format. - Kevin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36289/#review91793 --- On July 14, 2015, 6:13 p.m., Renan DelValle wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36289/ --- (Updated July 14, 2015, 6:13 p.m.) Review request for Aurora. 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
Re: Review Request 36289: Custom executor support for Scheduler
On Wed, Jul 15, 2015 at 12:13 PM, Kevin Sweeney kevi...@apache.org wrote: On July 15, 2015, 12:08 p.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java, lines 87-115 https://reviews.apache.org/r/36289/diff/2/?file=1011920#file1011920line87 In this change, we cannot remove these arguments as it breaks compatibility. In this change, we'll need to synthesize an entry as though it were read from the json file, and log a big warning about the deprecation. Can you elaborate on the compatibility breakage? IMO changing command-line args across releases doesn't rise to the level of a compatibility break, as long as the change is isolated to a single component. This doesn't change the way the scheduler behaves on the network from the perspective of other components. The deprecation warning could just as easily be an error, pointing to documentation for the new format. I was on the fence here, and decided to err on the conservative side. If this is the stance we want to take, i'm all for it. Updating my comment. - Kevin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36289/#review91793 --- On July 14, 2015, 6:13 p.m., Renan DelValle wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36289/ --- (Updated July 14, 2015, 6:13 p.m.) Review request for Aurora. 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
Re: Review Request 36289: Custom executor support for Scheduler
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36289/#review91793
---
Stopped at parsing code, since i belive it can be made quite a bit simpler.
src/dist/etc/executors.json (line 1)
https://reviews.apache.org/r/36289/#comment145442
Please move this file to `examples/vagrant`, as this configuration is not
suitable for distribution to others.
src/main/java/org/apache/aurora/scheduler/app/ExecutorSettingsLoader.java (line
57)
https://reviews.apache.org/r/36289/#comment145446
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;
SetString resources;
...
}
```
Then have gson read `ListExecutorConfig`.
We'll probably want to have one field that is something like `JsonObject`
so that the executor can have a snippet of custom schema.
src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java (lines 83 - 84)
https://reviews.apache.org/r/36289/#comment145448
In this change, we cannot remove these arguments as it breaks
compatibility. In this change, we'll need to synthesize an entry as though it
were read from the json file, and log a big warning about the deprecation.
- Bill Farner
On July 15, 2015, 1:13 a.m., Renan DelValle wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36289/
---
(Updated July 15, 2015, 1:13 a.m.)
Review request for Aurora.
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
Review Request 36289: Custom executor support for Scheduler
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36289/ --- Review request for Aurora. 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. 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 - build.gradle 78159932388046883494dda97b4a86c06772d26d examples/vagrant/upstart/aurora-scheduler.conf 1c2390f0248e91e65a548e67f6af1be8d2526b0a gradle/wrapper/gradle-wrapper.jar 085a1cdc27db1185342f15a00441734e74fe3735 gradle/wrapper/gradle-wrapper.properties e713793f7d63e65ea68e091b959ba6a68352b48f gradlew dd29ec5eae9a34d216475d6e5b5d8c810885aa76 src/dist/etc/executors.json PRE-CREATION src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 554a380bdb4ef69561259cdbfbc361694041571e src/main/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoader.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java 325f55640648151ae19e0c18c6961aeff10bfac3 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java c0d165ad34e46653dad95918e0058ebd3f2ee57f Diff: https://reviews.apache.org/r/36289/diff/ Testing --- ./gradlew test 895 tests completed, 44 failed, 3 skipped Thanks, Renan DelValle
Re: Review Request 36289: Custom executor support for Scheduler
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36289/#review90853 --- This patch does not apply cleanly on master (62a432a), do you need to rebase? I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On July 8, 2015, 2:06 a.m., Renan DelValle wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36289/ --- (Updated July 8, 2015, 2:06 a.m.) Review request for Aurora. 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. 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 - build.gradle 78159932388046883494dda97b4a86c06772d26d examples/vagrant/upstart/aurora-scheduler.conf 1c2390f0248e91e65a548e67f6af1be8d2526b0a gradle/wrapper/gradle-wrapper.jar 085a1cdc27db1185342f15a00441734e74fe3735 gradle/wrapper/gradle-wrapper.properties e713793f7d63e65ea68e091b959ba6a68352b48f gradlew dd29ec5eae9a34d216475d6e5b5d8c810885aa76 src/dist/etc/executors.json PRE-CREATION src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 554a380bdb4ef69561259cdbfbc361694041571e src/main/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoader.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java 325f55640648151ae19e0c18c6961aeff10bfac3 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java c0d165ad34e46653dad95918e0058ebd3f2ee57f Diff: https://reviews.apache.org/r/36289/diff/ Testing --- ./gradlew test 895 tests completed, 44 failed, 3 skipped Thanks, Renan DelValle
