aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Renan DelValle <rdelv...@binghamton.edu>
Subject Re: Review Request 50480: Multiple executor support in Scheduler
Date Thu, 28 Jul 2016 22:13:32 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message