> On Aug. 2, 2016, 7:36 p.m., Joshua Cohen wrote: > > I was about to push this change, but in the process of running e2e tests > > locally after applying your patch, it occurs to me that perhaps this is > > something we should have e2e coverage for. > > > > Do you think it would be feasible to add something to test_end_to_end.sh > > that verifies the multiple executor (and transitively the custom executor) > > support? If it would be a big change, I'd be ok with shipping it in a > > follow up review. > > Renan DelValle wrote: > This is a far point and one I considered as well. It would be of great > benefit to add a test but I think it might be a bigger change than it seems > on the surface. One of the biggest pain points to getting this done is being > able to submit tasks with a configurable ExecutorConfig.name using pystachio. > > For now the best we have technical support for is being able to load > multiple executors sucessfully which is one of the test for > ExecutorSettingsLoader. > > That said, I'll be looking into it and would gladly provide a follow up > patch. > > On a slight side note, since we'll be using ExecutorConfig.name to > identify the executor to use, the current default for thermos is > "aurora_executor" (as set on the thrift api). If we want to change this to > "thermos", now's the time to do it. (From what I've seen this constant was > not used anywhere in the Server side code until this patch.) > > Joshua Cohen wrote: > I'm ok with looking into e2e tests in a follow up patch. Would you mind > filing a ticket to track that? > > As far as changing the executor name goes, I'm neutral on that. I'm not > sure there's a huge upside, and there are potential downsides to it (e.g. > would it cause executor config to change for an otherwise no-op update)? > > Renan DelValle wrote: > No problem. > > Yeah, I'm neutral on this as well. The only benefit I see is being able > to test this feature, but I'm not sure about the ripple effects it may have. > > The alternative is using a custom client (which we're in the process of > preparing for release). The downsides here are obvious but it may be the > least painful option. We can continue discussion here: > https://issues.apache.org/jira/browse/AURORA-1744 > > Thanks again for looking over the code, very much appreciated.
Whoops sorry, crossed some cable here. Yeah, no huge upsides on changing the name on the Thrift API just wanted to bring it up in case anyone felt strongly about it. Will look into being able to set executor name from Pystachio to test this feature. - Renan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50480/#review144579 ----------------------------------------------------------- On Aug. 2, 2016, 4:38 p.m., Renan DelValle wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50480/ > ----------------------------------------------------------- > > (Updated Aug. 2, 2016, 4:38 p.m.) > > > Review request for Aurora, Joshua Cohen and Maxim Khutornenko. > > > Repository: aurora > > > Description > ------- > > Adds support for using multiple executors in a single scheduler. > > Added warnings for pre-emption performance degradation when increasing > executor overhead. > > Made executor config file require a JSON array. > > Modified the way in which Thermos and any custom executors are loaded at > startup. > > Thermos now always exists regardless of any other executors being used. > > Jobs sent where no executor configuration is found for them are rejected. > > Task prefix is now set from json file. > > Fixed non revokable resources not being filtered out from overhead. > > > Diffs > ----- > > RELEASE-NOTES.md 19d7f7e34652d170a7d8eb41a9c6ffdcc67324eb > docs/operations/configuration.md 0615c545f560597683f727c2425de4cc4e82c364 > src/main/java/org/apache/aurora/GuavaUtils.java > 8c2ab5720b808e5eae4c6b93fea8b787839b8dbe > src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java > c43e04aee0df8988ed3af9d463dd6747d6631e3b > > src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java > 93ed3600268f231b0e53ceb6b3674ff742d94407 > > src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorConfig.java > 24329a49155b9e8f6d11dd1d09f6f5188d1e2ad9 > > src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java > 8b7f8dcf4d8e0372208e636b3f57fc159272ecc7 > > src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettings.java > e919d3f2e2f86c26c0448029b06277a3a41a6941 > > src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettingsLoader.java > b74edf46d35ac99164ec1bcf33edf36556baf9ed > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java > f9b1c7cf30f93336fb850da09c1f2b7178cbdc17 > src/main/java/org/apache/aurora/scheduler/mesos/TestExecutorSettings.java > 1dfa97e659c2fca8308cb592f37d14328e4b42bc > > src/main/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilter.java > ee6fe95eaa41c7952a974ebea069b3de6ed82994 > src/main/java/org/apache/aurora/scheduler/scheduling/TaskScheduler.java > 3b84dbcfde9ae686110409173d742b3fa86ac764 > > src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java > 147327036a872c9ccd03e17daaaf8cb1df489843 > > src/test/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettingsLoaderTest.java > 7de7c46e6e1f478e25f7362d1d060b7f765dcb36 > > src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java > d34e12f6e0837fa43ea9b4e2a17db579e0ee10a2 > > src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java > bb8a849717a6ca3328ad4638acff5cc44ddd6ac9 > > src/test/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilterTest.java > 7eb1714d14581a6ab25e85d36a1f3e973380c536 > > src/test/java/org/apache/aurora/scheduler/scheduling/TaskSchedulerImplTest.java > fba427bd327e7f63b640c8b8753bfdeec3ee31e7 > src/test/java/org/apache/aurora/scheduler/thrift/Fixtures.java > a79b0f413e603374347f8ce5765fb6e71bc9a5b5 > > src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java > c0fe84371ff2f9d47721126a9c356180f7c166de > > src/test/resources/org/apache/aurora/scheduler/configuration/executor/test-missing-field.json > 464617028b1e765a563a3e11f70d66089ede6866 > > src/test/resources/org/apache/aurora/scheduler/configuration/executor/test-multiple-executor.json > PRE-CREATION > > src/test/resources/org/apache/aurora/scheduler/configuration/executor/test-single-executor.json > PRE-CREATION > > src/test/resources/org/apache/aurora/scheduler/configuration/executor/test-thermos-executor.json > 114eb4f6c3eaeca3ad976e4907a3ec32c2339a09 > > Diff: https://reviews.apache.org/r/50480/diff/ > > > Testing > ------- > > ./build-support/jenkins/build.sh > > ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > > Created and killed jobs running a custom executor using a custom client. > > Created and killed thermos jobs with the Aurora Client. > > > Thanks, > > Renan DelValle > >
