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