aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Maxim Khutornenko" <ma...@apache.org>
Subject Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.
Date Fri, 20 Feb 2015 23:58:14 GMT


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


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