aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bill Farner" <wfar...@apache.org>
Subject Re: Review Request 31821: Making preemptor asynchronous. Part 1 - extracting slot finder.
Date Sat, 07 Mar 2015 17:30:06 GMT

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


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