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 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.
Date Thu, 19 Feb 2015 22:05:52 GMT

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


Looks good!  Only a few minor suggestions.


src/main/java/org/apache/aurora/scheduler/async/OfferManager.java
<https://reviews.apache.org/r/30891/#comment119445>

    It would be helpful to include a comment about why things land in this map, if only to
save the reader from tracing what goes in.
    
    Also, please TODO+ticket exposing this map via a debug endpoint.



src/main/java/org/apache/aurora/scheduler/async/OfferManager.java
<https://reviews.apache.org/r/30891/#comment119450>

    Some debug visibility could be _really_ helpful here.
    
    Consider a `LOG.isLoggable(Level.FINE)` guard + `LOG.fine()` here and in `addStaticGroupBan`.



src/main/java/org/apache/aurora/scheduler/async/OfferManager.java
<https://reviews.apache.org/r/30891/#comment119454>

    It wouldn't hurt to ensure consistency between `staticallyBannedOffers` and `offersById`
by making sure that the offer ID exists in `offersById` before adding to this map.  It's plausible
for `launchFirst` to race removal of an offer, resulting in a memory leak in `staticallyBannedOffers`.



src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java
<https://reviews.apache.org/r/30891/#comment119447>

    More context would be helpful here, specifically about what a static mismatch is.  (Feel
free to link to `VetoGroup#STATIC`.



src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java
<https://reviews.apache.org/r/30891/#comment119449>

    I find this more readable:
    
        if (Veto.identifyGroup(vetoes) == VetoGroup.STATIC) {
          return Result.FAILURE_STATIC_MISMATCH;
        } else {
          return Result.FAILURE;
        }
    
    Or, if you prefer ternary:
    
        return (Veto.identifyGroup(vetoes) == VetoGroup.STATIC)
            ? Result.FAILURE_STATIC_MISMATCH
            : Result.FAILURE;


- Bill Farner


On Feb. 18, 2015, 2:40 a.m., Maxim Khutornenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30891/
> -----------------------------------------------------------
> 
> (Updated Feb. 18, 2015, 2:40 a.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-909
>     https://issues.apache.org/jira/browse/AURORA-909
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Offer filtering for static vetoes. Part 3 of 4: Filtering out statically banned offers.
> 
> Will not apply cleanly: diffed with https://reviews.apache.org/r/30890 as a parent.
> 
> Original RB: https://reviews.apache.org/r/28617/
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/async/OfferManager.java b241d7b22c3d1ceca127b2671eb608ae41283bf3

>   src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 21ea7d2b9d2f8c76367d7ae985270402bb51ea26

>   src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 5a0f7ddb7e8fa6869cbb0fdfd07c6881780c6917

>   src/test/java/org/apache/aurora/scheduler/async/OfferManagerImplTest.java 7ee2bb9bec6c59ba67b65d5b1229c64aca1277ff

>   src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 9eef52a333e09454e8fd0026371c7e64472a883d

>   src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java b6d4d8e771c7d16a46e43c7d5c427b911f8b661d

> 
> Diff: https://reviews.apache.org/r/30891/diff/
> 
> 
> Testing
> -------
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>


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