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



src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java
<https://reviews.apache.org/r/32597/#comment130383>

    ```
    Set<String> allSlaves = Sets.newHashSet(Iterables.concat(
        slavesToOffers.keySet(),
        slavesToActiveTasks.keySet()));
    ```



src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java
<https://reviews.apache.org/r/32597/#comment130384>

    "Reservations are made for at most one task group per slave."
    
    This reads as though you might make multiple reservations on a slave, so 
long as they are from different task groups.



src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java
<https://reviews.apache.org/r/32597/#comment130385>

    "neither of the slaves"  what does that mean?



src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java
<https://reviews.apache.org/r/32597/#comment130388>

    It's probably not worth changing code, but might be worth noting that this 
breaks round-robin, since the position is reset.



src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java
<https://reviews.apache.org/r/32597/#comment130393>

    Given that this is all private/local code, consider 
`HashMultiset.create(Iterable)` here and avoid the immutable copy -> mutable 
copy dance.



src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java
<https://reviews.apache.org/r/32597/#comment130404>

    Less efficient when there are many task groups, but i find this easier to 
read:
    
    ```
    List<TaskGroupKey> instructions = Lists.newLinkedList();
    Set<TaskGroupKey> keys = ImmutableSet.copyOf(groups.elementSet());
    while (!groups.isEmpty()) {
      for (TaskGroupKey key : keys) {
        if (groups.contains(key)) {
          instructions.add(key);
          // Removes a single instance of key.
          groups.remove(key);
        }
      }
    }
    ```



src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionProposal.java
<https://reviews.apache.org/r/32597/#comment130405>

    a small doc would be nice here



src/main/java/org/apache/aurora/scheduler/async/preemptor/Preemptor.java
<https://reviews.apache.org/r/32597/#comment130406>

    `slaveId`
    
    
https://google-styleguide.googlecode.com/svn/trunk/javaguide.html#s5.3-camel-case



src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessorTest.java
<https://reviews.apache.org/r/32597/#comment130413>

    Should have brought this up in a previous review, but i'm uncomfortable 
with using a mock to control the behavior of a data structure.  It really feels 
like the test knows way too much about the internals of the class at this 
point, given that this is internally-managed state.  Is there any particular 
reason we shouldn't use a concrete `BiCache` instance here?



src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessorTest.java
<https://reviews.apache.org/r/32597/#comment130414>

    `@Nullable String slaveId`



src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessorTest.java
<https://reviews.apache.org/r/32597/#comment130415>

    I'd like to avoid randomness in tests, as trivial as it may seem here.  
It's reasonable to assume that calling `makeTask()` with identical arguments 
should at least produce an `equals()` object.  I'd rather see a task id 
argument to make that true.



src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImplTest.java
<https://reviews.apache.org/r/32597/#comment130417>

    I don't think anyObject() is what you want, that's for matching arguments.  
This returns `null`.



src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImplTest.java
<https://reviews.apache.org/r/32597/#comment130427>

    Related to the `OfferManager.andReturn` mock above, you could return a 
constant there, and expect the same constant here.  Then you get to drop all 
the `eq()`s.


- Bill Farner


On April 16, 2015, 1:39 a.m., Maxim Khutornenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32597/
> -----------------------------------------------------------
> 
> (Updated April 16, 2015, 1:39 a.m.)
> 
> 
> Review request for Aurora, Bill Farner and Zameer Manji.
> 
> 
> Bugs: AURORA-1219
>     https://issues.apache.org/jira/browse/AURORA-1219
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> This is addressing the preemption efficiency loss due to making preemptor 
> async. Summary:
> - Implemented fair task processing by evaluating pending task groups in 
> round-robin fashion (e.g.: {G1:3, G2:2} -> {G1, G2, G1, G2, G1}) against all 
> available slaves.
> - Moved relevant functionality from PreemptionSlotFinder (now 
> PreemptionVictimFilter) into PendingTaskProcessor.
> 
> The bulk of new functionality is in PendingTaskIterator and 
> PendingTaskProcessor. The rest is refactoring to adjust to the new traversal 
> approach.
> 
> 
> Diffs
> -----
> 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java
>  00919b7910704c5025465e1071378a978e5e60a3 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionProposal.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java
>  f16f964f56f0f9da523950891293083f1bd86780 
>   src/main/java/org/apache/aurora/scheduler/async/preemptor/Preemptor.java 
> 5200811ec8fe6fedf42ae2712f29051a8c0bf4a9 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorMetrics.java
>  dc7eb4421ff305dca32f36c83605c2864fea8b11 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModule.java
>  7cea881a8c3c11142bd04b3c794cd86a310b15e7 
>   src/main/java/org/apache/aurora/scheduler/base/TaskGroupKey.java 
> 6af3949b85297043640edccc1a490906c0fcff6c 
>   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
> 7bb1e7a9f27088636d6549c089b1d079dfeaf2ee 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessorTest.java
>  8a9a3b7d9686e29632f4e267f591cdb19826e0e7 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImplTest.java
>  64283fab8c61b841007d7c0a02b083b3067bc78d 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorSlotFinderTest.java
>  eed2de99a145dd2124b7f2b4d401214f1d8adf2e 
> 
> Diff: https://reviews.apache.org/r/32597/diff/
> 
> 
> Testing
> -------
> 
> ./gradlew -Pq build
> Manual testing in vagrant
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Reply via email to