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

Ship it!



src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java
<https://reviews.apache.org/r/31821/#comment122841>

    Can you leave a TODO(wfarner) to reuse existing modules here?  The DRY 
violation here is only going to get worse until we do.



src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java
<https://reviews.apache.org/r/31821/#comment122842>

    Careful about using 'priority' here, as it can be confused for the field 
within `TaskConfig`.  With that in mind, you might move this doc to 
`PreemptorImpl`, as it's more of an implementation definition than interface.



src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java
<https://reviews.apache.org/r/31821/#comment122843>

    This makes for an interesting opportunity - this bit of data could be 
generalized to apply to any scheduling (whether or not preemption is required). 
 Nothing to do with this now, but you could imagine abstracting all scheduling 
decisions to fit behind this facade at some point.



src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java
<https://reviews.apache.org/r/31821/#comment122844>

    Nothing forces the backing Set to be immutable, consider changing the 
constructor to accept `ImmutableSet` to bolster immutability.



src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java
<https://reviews.apache.org/r/31821/#comment122845>

    s/final //



src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java
<https://reviews.apache.org/r/31821/#comment122846>

    Aha!  I knew i missed this in a previous patch.  We no longer have 
tri-state logic here (we wither return Optional.absent or a non-empty Set), so 
you can simplify this comment.



src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java
<https://reviews.apache.org/r/31821/#comment122847>

    Not yours, but it seems odd that we would call a function and internally 
suppress its behavior based on configuration.  Seems like the caller should 
avoid the call if the behavior is not desired.  If you agree, please TODO.



src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java
<https://reviews.apache.org/r/31821/#comment122848>

    s/static //
    
    Also not yours, but wording is confusing here.  Specifically, when i read 
"the provided task" i think of the `pendingTask` parameter, but it's actually 
the opposite.  How about:
    
    ```
    Creates a filter that will find tasks that the provided {@code pendingTask} 
may preempt.
    ```



src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImpl.java
<https://reviews.apache.org/r/31821/#comment122849>

    I'm not sure if this is the correct spot, but consider leaving a TODO to 
validate consistency of the (possibly weakly-consistent) preemption proposal.



src/test/java/org/apache/aurora/scheduler/async/preemptor/TestUtil.java
<https://reviews.apache.org/r/31821/#comment122850>

    I'm highly skeptical of the shelf life of test fixtures like this.  Here we 
have 6 primitive parameters, making it very easy to accidentally swap them at 
the call site.  It's slightly different when the utility is within a unit test, 
but sharing only accelerates towards an unmaintainable state.
    
    I suggest you either minimally duplicate these helpers in the two unit test 
classes, or trim down the parameters to something more universal like:
    
    ```
    makeTask(IJobKey job, String id)
    ```
    
    and make the caller mutate the result for any additional customization.
    
    Consider this as a TODO for now, so we can avoid further modification of 
the test cases.  However, in this diff please correct the line wrap style on 
the arguments.


- Bill Farner


On March 7, 2015, 1:36 a.m., Maxim Khutornenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31821/
> -----------------------------------------------------------
> 
> (Updated March 7, 2015, 1:36 a.m.)
> 
> 
> Review request for Aurora, Bill Farner and Zameer Manji.
> 
> 
> Bugs: AURORA-1158
>     https://issues.apache.org/jira/browse/AURORA-1158
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Extracting PreemptorSlotFinder to be reused for slot validation in later 
> stages. The changes are very minimal and mostly around metric handling and 
> test code.
> 
> Also added missing test coverage.
> 
> 
> Diffs
> -----
> 
>   src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java 
> 701b9052696337766cb233c865cb9fbb4907071e 
>   src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 
> e093ca54521ffb9399bb97ce60f510331af70853 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java
>  PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/async/preemptor/Preemptor.java 
> bddb9647493b3e7a58c40d4b477a06161c1388a2 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImpl.java 
> ae56d1e09322869eedd7a27586cd6f96edd64e0a 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorMetrics.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModule.java
>  85b3874a36ed07c684f26da172952c932cff707a 
>   src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 
> 58733bdc4dd6de29ccead5cb0a267286e8dc0656 
>   src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 
> 891cc098cca99e84ba014b7131106ceb0b429b5f 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImplTest.java
>  83680769611878886da04e1794b321aa1986e678 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModuleTest.java
>  020b67187a18bba64d9b562c3a6c0969fc85d469 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorSlotFinderTest.java
>  PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/async/preemptor/TestUtil.java 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/31821/diff/
> 
> 
> Testing
> -------
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Reply via email to