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


Partial review - stopped at the question of how to ensure the precondition 
check added in SchedulingFilterImpl always passes. I think this patch needs to 
add startup validation and a backfill strategy.


api/src/main/thrift/org/apache/aurora/gen/api.thrift (line 46)
<https://reviews.apache.org/r/36289/#comment151027>

    Hmm - why does this magic string exist? Any idea what the implications of 
changing it are?



examples/vagrant/executors-config.json (lines 5 - 6)
<https://reviews.apache.org/r/36289/#comment151028>

    Is there a difference between path and resources as far as Mesos is 
concerned?



src/main/java/org/apache/aurora/scheduler/app/ExecutorConfiguration.java (line 
34)
<https://reviews.apache.org/r/36289/#comment151037>

    Given that you use this type in a `Set` you will need to define `hashCode` 
and `equals` here (and for all contained types).



src/main/java/org/apache/aurora/scheduler/app/ExecutorConfiguration.java (line 
37)
<https://reviews.apache.org/r/36289/#comment151034>

    Make these `final` and add a constructor for testing?



src/main/java/org/apache/aurora/scheduler/app/ExecutorConfiguration.java (line 
48)
<https://reviews.apache.org/r/36289/#comment151038>

    Needs `hashCode` and `equals`



src/main/java/org/apache/aurora/scheduler/app/ExecutorConfiguration.java (line 
85)
<https://reviews.apache.org/r/36289/#comment151031>

    remove redundant use of `this`, here and below



src/main/java/org/apache/aurora/scheduler/app/ExecutorSettingsLoader.java 
(lines 40 - 44)
<https://reviews.apache.org/r/36289/#comment151036>

    No JavaDoc for private methods - use a comment instead:
    
    ```java
    private ExecutorSettingsLoader() {
      // Utility class.
    }
    ```



src/main/java/org/apache/aurora/scheduler/app/ExecutorSettingsLoader.java (line 
60)
<https://reviews.apache.org/r/36289/#comment151035>

    Take a `File` here instead of a `String`.
    
    Return a narrower type, like `Set<ExecutorSettings>` (but probably 
`List<ExecutorSetttings>`, see below.



src/main/java/org/apache/aurora/scheduler/app/ExecutorSettingsLoader.java (line 
63)
<https://reviews.apache.org/r/36289/#comment151039>

    Prefer `ImmutableSet.Builder`



src/main/java/org/apache/aurora/scheduler/app/ExecutorSettingsLoader.java (line 
66)
<https://reviews.apache.org/r/36289/#comment151040>

    You leak a file descriptor here - consider just using 
`Files.asCharSource(configFile, StandardCharsets.UTF_8)` and opening it in a 
try-with-resources block:
    
    ```java
    CharSource configFileSource = Files.asCharSource(configFile, 
StandardCharsets.UTF_8);
    try (Reader reader = configFileSource.openBufferedReader()) {
      gson.read(...);
    }
    ```



src/main/java/org/apache/aurora/scheduler/app/ExecutorSettingsLoader.java (line 
68)
<https://reviews.apache.org/r/36289/#comment151047>

    nit: no space between `}` and `.`



src/main/java/org/apache/aurora/scheduler/app/ExecutorSettingsLoader.java (line 
75)
<https://reviews.apache.org/r/36289/#comment151046>

    Mentioning the filename would make this easier to debug.



src/main/java/org/apache/aurora/scheduler/app/ExecutorSettingsLoader.java 
(lines 76 - 77)
<https://reviews.apache.org/r/36289/#comment151044>

    This is actually caused by the platform JVM not having support for an 
encoding (which is impossible for UTF8 as it's required by the standard). Use 
the `asCharSource` suggestion with the constant from `StandardCharsets` to 
avoid needing to handle this.



src/main/java/org/apache/aurora/scheduler/app/ExecutorSettingsLoader.java (line 
79)
<https://reviews.apache.org/r/36289/#comment151045>

    You'll want to concat the error message here as well as the second param 
will just print the stack trace. Also a bit more context would be useful. e.g.
    
    ```java
    throw new ExecutorSettingsException("Error parsing executor settings JSON: 
" + e, e);
    ```



src/main/java/org/apache/aurora/scheduler/app/ExecutorSettingsLoader.java (line 
81)
<https://reviews.apache.org/r/36289/#comment151041>

    Can you determine which parameter is missing? This would be a very 
frustrating error message to debug.



src/main/java/org/apache/aurora/scheduler/app/ExecutorSettingsLoader.java (line 
84)
<https://reviews.apache.org/r/36289/#comment151048>

    You read this as a `List`, any reason not to use `ImmutableList.copyOf` 
here instead?



src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java (line 81)
<https://reviews.apache.org/r/36289/#comment151049>

    `Arg<File>` and add a `@CanRead` annotation for more useful validation.



src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java (line 170)
<https://reviews.apache.org/r/36289/#comment151050>

    This needs to re-throw, otherwise initialization will continue and users 
will experience weird runtime behavior. Consider removing the custom exception 
type and throwing `IllegalArgumentException` in the parser.



src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java 
(lines 211 - 213)
<https://reviews.apache.org/r/36289/#comment151051>

    Formatting is weird - push the call chain to the next line:
    
    ```java
    ResourceSlot.from(request.getTask())
        
.withOverhead(requireNonNull(executorSettings.get(task.getExecutorConfig().getName());
    ```
    
    This is a weird (though correct) place for a `requireNonNull` check though 
because the scheduler can crash in its scheduling loop in the case of a config 
change.
    
    I think you need to add logic to make sure that every task has an executor 
that's acceptible to the system at startup.


- Kevin Sweeney


On Aug. 17, 2015, 11:10 p.m., Renan DelValle wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36289/
> -----------------------------------------------------------
> 
> (Updated Aug. 17, 2015, 11:10 p.m.)
> 
> 
> Review request for Aurora and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> What was done:
> ==============
> Added support for dynamically chosing an executor that's definied in a server 
> side config file.
> Removed command line arguments that were moved over to the config file.
> Updated existing code to reflect the use of a Map instead of a single 
> ExecutorSettings object.
> 
> Future:
> =======
> Create an offshoot of the current client that allows to send thrift calls 
> with different executor configs which will allow use of custom executors. 
> Some work on this has already been done and will be published ASAP for 
> testing.
> 
> 
> Diffs
> -----
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> f792be0ad393072b4a4ec525363e06cfd16b63d0 
>   examples/vagrant/executors-config.json PRE-CREATION 
>   examples/vagrant/upstart/aurora-scheduler-kerberos.conf 
> 744b4a35c61e749734e222b3d4cbd296927665aa 
>   examples/vagrant/upstart/aurora-scheduler.conf 
> 789a3a0315e8530880999432aa9b1e7d0f57d1ff 
>   src/main/java/org/apache/aurora/scheduler/ResourceSlot.java 
> e5953bbf02fc2b08fbdff5c25b5389c5a209dfca 
>   src/main/java/org/apache/aurora/scheduler/app/ExecutorConfiguration.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/app/ExecutorSettingsLoader.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> e74b36bc5f85e5ae5fbb2e0b1e34961251739d9e 
>   src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java 
> 10389640c87b203386313ab79204ea936272d350 
>   src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java 
> b3c913892248e4a9a8111412307463985f5ca97f 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
> ff6eb980292c05e35dcf68104c870a7bef95629a 
>   src/main/java/org/apache/aurora/scheduler/preemptor/PreemptionVictim.java 
> 8162323816aedc711a3af84cd499250b78718ab3 
>   
> src/main/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilter.java
>  a0e71e1c74f67b8836e7da5418012f342977f661 
>   src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 
> 50e7fc91108993e547869df5b9e5c925fb89a225 
>   
> src/test/java/org/apache/aurora/scheduler/app/ExecutorConfigurationTest.java 
> PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/app/ExecutorSettingsLoaderTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
> 37772d0b75d022f072af10d82d096981680e193f 
>   
> src/test/java/org/apache/aurora/scheduler/events/NotifyingSchedulingFilterTest.java
>  608af1afe6fc27c8c597490e88fed75580076c95 
>   
> src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java
>  b2327a47374d81b59886c1e4575ded8340322db7 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
> 02fe96445148d1e14d85dc7a6fa386d84a8a8c70 
>   src/test/java/org/apache/aurora/scheduler/mesos/TaskExecutors.java 
> 6a80503aeb2058e8f8065285adc151197d2d14d6 
>   
> src/test/java/org/apache/aurora/scheduler/preemptor/ClusterStateImplTest.java 
> a1ac922d471013779710e02c0c9ca9f84b506807 
>   
> src/test/java/org/apache/aurora/scheduler/preemptor/PendingTaskProcessorTest.java
>  b9cb5bfe9f89a8bfdb96b6eeb1998ed105963484 
>   
> src/test/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilterTest.java
>  66f20c6a63b331353c467cde5521f21e4df49e2d 
>   
> src/test/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimTest.java 
> 09380f95a7d9405f770513db35d2a45d23d89b61 
>   src/test/java/org/apache/aurora/scheduler/preemptor/PreemptorImplTest.java 
> b07ff7babd217dac4153831a0d78325bcb72b306 
>   
> src/test/resources/org/apache/aurora/scheduler/app/executor-settings-example.json
>  PRE-CREATION 
>   
> src/test/resources/org/apache/aurora/scheduler/app/executor-settings-mesos-command-example.json
>  PRE-CREATION 
>   
> src/test/resources/org/apache/aurora/scheduler/app/executor-settings-thermos-no-observer.json
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/36289/diff/
> 
> 
> Testing
> -------
> 
> Ran jenkins build test, passed all tests, code style checks, findbugs check, 
> and PMD.
> Ran end to end, failed upon reaching kerberos tests.
> 
> 
> Thanks,
> 
> Renan DelValle
> 
>

Reply via email to