> On Feb. 19, 2015, 10:05 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/async/OfferManager.java, line 295 > > <https://reviews.apache.org/r/30891/diff/5/?file=867191#file867191line295> > > > > Some debug visibility could be _really_ helpful here. > > > > Consider a `LOG.isLoggable(Level.FINE)` guard + `LOG.fine()` here and > > in `addStaticGroupBan`.
Done. > On Feb. 19, 2015, 10:05 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/async/OfferManager.java, line 299 > > <https://reviews.apache.org/r/30891/diff/5/?file=867191#file867191line299> > > > > 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`. Done. > On Feb. 19, 2015, 10:05 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java, line 63 > > <https://reviews.apache.org/r/30891/diff/5/?file=867193#file867193line63> > > > > More context would be helpful here, specifically about what a static > > mismatch is. (Feel free to link to `VetoGroup#STATIC`. Linked to VetoGroup. > On Feb. 19, 2015, 10:05 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java, line 117 > > <https://reviews.apache.org/r/30891/diff/5/?file=867193#file867193line117> > > > > 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; Done. > On Feb. 19, 2015, 10:05 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/async/OfferManager.java, line 251 > > <https://reviews.apache.org/r/30891/diff/5/?file=867191#file867191line251> > > > > 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. Done and done. - Maxim ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/#review73208 ----------------------------------------------------------- On Feb. 20, 2015, 11:58 p.m., Maxim Khutornenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30891/ > ----------------------------------------------------------- > > (Updated Feb. 20, 2015, 11:58 p.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 > >
