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 Wed, 27 Jul 2016 21:05:38 GMT


> On July 26, 2016, 8:25 p.m., Joshua Cohen wrote:
> > CHANGELOG, lines 1-4
> > <https://reviews.apache.org/r/50480/diff/1/?file=1454687#file1454687line1>
> >
> >     This file is updated automatically by our release script. You can revert these
changes.

Got it. Was a bit confused because last time my change didn't go into it, but it might have
to do with the ticket not having been closed at the right time.


> On July 26, 2016, 8:25 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java,
line 166
> > <https://reviews.apache.org/r/50480/diff/1/?file=1454692#file1454692line166>
> >
> >     Kill this line?

Good catch, thanks!


> On July 26, 2016, 8:25 p.m., Joshua Cohen 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>
> >
> >     Fix indentation. Style for continuation indents should be:
> >     
> >         public ExecutorSettings(
> >             ImmutableMap<...> config,
> >             boolean populateDiscoveryInfo) {
> >             
> >           // code continues here after blank line above

Done.


> On July 26, 2016, 8:25 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettings.java,
lines 40-45
> > <https://reviews.apache.org/r/50480/diff/1/?file=1454693#file1454693line40>
> >
> >     Instead of creating separate methods for get config and checking a config's
existence, how about changing `getExecutorConfig` to return `Optional<ExecutorConfig>`
and using `Optional#isPresent` as the indicator of whether config exists for that name?

Great suggestion, thanks!


> On July 26, 2016, 8:25 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettingsLoader.java,
line 82
> > <https://reviews.apache.org/r/50480/diff/1/?file=1454694#file1454694line82>
> >
> >     House style is: `Map<K, V> foo = Maps.newHashMap()`. It's a legacy of
the project's pre-Java 7 origins. In theory at some point we could just move to Java 7 diamond
syntax (`... = new HashMap<>()`), but until that time, best to remain consistent.

Gotcha, will keep this in mind for future patches.


> On July 26, 2016, 8:25 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettingsLoader.java,
lines 97-98
> > <https://reviews.apache.org/r/50480/diff/1/?file=1454694#file1454694line97>
> >
> >     multiExecutors.put(
> >             e.getName(),
> >             new ExecutorConfig(...))

Fixed.


> On July 26, 2016, 8:25 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java, line 230
> > <https://reviews.apache.org/r/50480/diff/1/?file=1454695#file1454695line230>
> >
> >     Instead of passing in the task, just to use it to get the executor name, why
not just pass in the executor name?

Fixed.


> On July 26, 2016, 8:25 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java, line 271
> > <https://reviews.apache.org/r/50480/diff/1/?file=1454695#file1454695line271>
> >
> >     Same here.

For this one, I was passing the task because a valid Docker task may or may not have an executor
config. I agree though, and an Optional is more suitable here. I'll go ahead and make that
change.


> On July 26, 2016, 8:25 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/scheduling/TaskScheduler.java, lines 141-143
> > <https://reviews.apache.org/r/50480/diff/1/?file=1454698#file1454698line141>
> >
> >     Should fit on one line?

Yep! Fixed.


> On July 26, 2016, 8:25 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/scheduling/TaskScheduler.java, line 158
> > <https://reviews.apache.org/r/50480/diff/1/?file=1454698#file1454698line158>
> >
> >     Are we guaranteed that the executor config exists here? The previous branch
is checking if executorConfig is set and that the config exists, but won't we fall into this
branch if executorConfig is not set?

Great catch! I forgot to put in some logic to handle the case where it's a Docker task with
no executor config.
Iffy on how the code on this should look like without making it convoluted because there are
a couple of scenarios that need to be handled:

1. Task with Docker container and with executor config -> must have a valid config
2. Task without docker container and with executor config -> must have a valid config
3. Task with Docker container and without executor config -> Accept
4. Task without docker container and without executor config -> Reject

Another question that comes to mind here is, if there is if there's a task with a docker container
but no executor config set, should the task be allocated thermo's overhead or not.


> On July 26, 2016, 8:25 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettingsLoader.java,
line 100
> > <https://reviews.apache.org/r/50480/diff/1/?file=1454694#file1454694line100>
> >
> >     We should probably return an `ImmutableMap` here.

Agreed, fixed.


- Renan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50480/#review143664
-----------------------------------------------------------


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