> On July 27, 2016, 1:47 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilter.java, > > lines 205-208 > > <https://reviews.apache.org/r/50480/diff/1/?file=1454697#file1454697line205> > > > > I'd drop this in favor of EMPTY overhead already returned by the method > > call below. It would leave ConfigurationManager and TaskScheduler to assert > > both new and existing task routes.
Sounds good to me, applied the change. > On July 27, 2016, 1:47 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettings.java, > > lines 33-34 > > <https://reviews.apache.org/r/50480/diff/1/?file=1454693#file1454693line33> > > > > We usually avoid concrete collection types in publicly accessible > > methods/constructors. Accepting Map should be enough. > > > > If you worry about modifications (which does not seem to be the case > > here) you can do ImmutableMap.copyOf() in the assignment. Makes sense, fixed. > On July 27, 2016, 1:47 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java, > > lines 113-120 > > <https://reviews.apache.org/r/50480/diff/1/?file=1454695#file1454695line113> > > > > How about moving EXECUTOR_PREFIX into ExecutorSettings and make it > > fully configurable instead? The default would be the current 'thermos-' > > value overriden by custom executor definition. Great suggestion, implemented. > On July 27, 2016, 1:47 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java, > > lines 162-164 > > <https://reviews.apache.org/r/50480/diff/1/?file=1454695#file1454695line162> > > > > Is this reachable? Looks like you return Optional.fromNullable() from > > that method instead of throwing. > > > > How about > > `executorSettings.getExecutorOverhead(...).orElse(ResourceBag.EMPTY)` > > instead? Throwing exception this late in the scheduling cycle does not seem > > right anyway. That sounds like a good idea. I agree that by this point in the pipeline, we should have validated everything. > On July 27, 2016, 1:47 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilter.java, > > line 122 > > <https://reviews.apache.org/r/50480/diff/1/?file=1454697#file1454697line122> > > > > This is actually a great finding! Should not be hard to fix it in this > > diff and avoid the TODO, e.g.: > > ``` > > ResourceBag overhead = > > executorSettings.getExecutorOverhead(...).orElse(EMPTY); > > if (tierManager.getTier(victim.getConfig()).isRevocable()) { > > bag = > > bag.filter(IS_MESOS_REVOCABLE.negate()).add(overhead.filter(IS_MESOS_REVOCABLE.negate())); > > } else { > > bag.add(overhead); > > } > > ``` Meant to ask about this but forgot to. Glad I left a todo in there as a reminder. Fixed in this diff. > On July 27, 2016, 1:47 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/scheduling/TaskScheduler.java, > > lines 139-148 > > <https://reviews.apache.org/r/50480/diff/1/?file=1454698#file1454698line139> > > > > I suggest convert this block into an assert statement and move into > > current else block: > > > > ``` > > if (!executorSettings.getExecutorConfig(...).isPresent()) { > > throw new IllegalStateException("Cannot find executor > > configuration..."); > > } > > ``` Makes sense, changed, thanks for the suggestion. Wasn't sure if it was OK to throw an exception from here. - Renan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50480/#review143749 ----------------------------------------------------------- On July 26, 2016, 7:04 p.m., Renan DelValle wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50480/ > ----------------------------------------------------------- > > (Updated July 26, 2016, 7:04 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. > > > Diffs > ----- > > CHANGELOG fc6a46d77ebf889c8f60402c95e8a7472980ba1c > RELEASE-NOTES.md d46420ef876b88d9ab68c4816f1c2d6780aba702 > docs/operations/configuration.md 0615c545f560597683f727c2425de4cc4e82c364 > 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/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 > cbbd6be94aa857b02cd7b45bfb2f0216d9a1cec3 > 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 > 500fd435b4c72b25abd8df7eea6b3850edc96e99 > > 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 > > > Thanks, > > Renan DelValle > >
