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