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




CHANGELOG (lines 1 - 4)
<https://reviews.apache.org/r/50480/#comment209522>

    This file is updated automatically by our release script. You can revert 
these changes.



src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java
 (line 165)
<https://reviews.apache.org/r/50480/#comment209523>

    Kill this line?



src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettings.java
 (lines 33 - 34)
<https://reviews.apache.org/r/50480/#comment209524>

    Fix indentation. Style for continuation indents should be:
    
        public ExecutorSettings(
            ImmutableMap<...> config,
            boolean populateDiscoveryInfo) {
            
          // code continues here after blank line above



src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettings.java
 (lines 40 - 45)
<https://reviews.apache.org/r/50480/#comment209527>

    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?



src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettingsLoader.java
 (line 61)
<https://reviews.apache.org/r/50480/#comment209525>

    s/An map/A map



src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettingsLoader.java
 (line 82)
<https://reviews.apache.org/r/50480/#comment209526>

    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.



src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettingsLoader.java
 (lines 97 - 98)
<https://reviews.apache.org/r/50480/#comment209528>

    multiExecutors.put(
            e.getName(),
            new ExecutorConfig(...))



src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettingsLoader.java
 (line 100)
<https://reviews.apache.org/r/50480/#comment209529>

    We should probably return an `ImmutableMap` here.



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (line 230)
<https://reviews.apache.org/r/50480/#comment209530>

    Instead of passing in the task, just to use it to get the executor name, 
why not just pass in the executor name?



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (line 271)
<https://reviews.apache.org/r/50480/#comment209531>

    Same here.



src/main/java/org/apache/aurora/scheduler/scheduling/TaskScheduler.java (lines 
141 - 143)
<https://reviews.apache.org/r/50480/#comment209532>

    Should fit on one line?



src/main/java/org/apache/aurora/scheduler/scheduling/TaskScheduler.java (line 
158)
<https://reviews.apache.org/r/50480/#comment209533>

    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?


- Joshua Cohen


On July 27, 2016, 2:04 a.m., Renan DelValle wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50480/
> -----------------------------------------------------------
> 
> (Updated July 27, 2016, 2:04 a.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
> 
>

Reply via email to