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 Tue, 24 Feb 2015 21:29:57 GMT


> On Feb. 24, 2015, 7:35 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/async/OfferManager.java, line 301
> > <https://reviews.apache.org/r/30891/diff/7/?file=873086#file873086line301>
> >
> >     No need to hold the intrinsic lock while logging here

Having an explicit lock will be inconsistent with the rest of this class and will add up to
clutter. Given the on-demand logging here, I don't see much value in over-optimizing this
logic.


> On Feb. 24, 2015, 7:35 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/async/OfferManager.java, line 317
> > <https://reviews.apache.org/r/30891/diff/7/?file=873086#file873086line317>
> >
> >     Same as above - no need to hold the intrinsic lock while logging here.

Same here.


> On Feb. 24, 2015, 7:35 p.m., Kevin Sweeney wrote:
> > src/test/java/org/apache/aurora/scheduler/async/OfferManagerImplTest.java, line
78
> > <https://reviews.apache.org/r/30891/diff/7/?file=873089#file873089line78>
> >
> >     It would be good form to reset the level in the tearDown here.

Sure, done.


> On Feb. 24, 2015, 7:35 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java, line 119
> > <https://reviews.apache.org/r/30891/diff/3/?file=862800#file862800line119>
> >
> >     as written, a caller needs to know FAILURE and FAILURE_STATIC_MISMATCH both
mean failure, but it would be easy to just check for `== FAILURE` and miss that in code reviews.
Would it make sense to add `isFailure()` and `isStaticMismatch()` here?
> >     
> >     Also, consider fetching the VetoGroup in the constructor so that precondition
checks will fail faster

>as written, a caller needs to know FAILURE and FAILURE_STATIC_MISMATCH both mean failure,
but it would be easy to just check for == FAILURE and miss that in code reviews. Would it
make sense to add isFailure() and isStaticMismatch() here?

This would shift failure detection responsibility upstream and will open up for invalid combinations
like isFailure=False & isStaticMismatch=True. I believe enum-based approach is the more
accurate and easier to reason solution in this case.

>Also, consider fetching the VetoGroup in the constructor so that precondition checks will
fail faster

`Veto.identifyGroup` does not provide any additional error handling. It's already failing
fast as the only way to create an instance with vetoes is through this:
```
 public static Assignment failure(Set<Veto> vetoes) {
   return new Assignment(NO_TASK_INFO, MorePreconditions.checkNotBlank(vetoes));
 }
```


- Maxim


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


On Feb. 23, 2015, 8:36 p.m., Maxim Khutornenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30891/
> -----------------------------------------------------------
> 
> (Updated Feb. 23, 2015, 8:36 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