> On April 15, 2015, 7:58 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java, > > line 140 > > <https://reviews.apache.org/r/32597/diff/4/?file=930944#file930944line140> > > > > How about > > ``` > > ImmutableSet.copyOf(Sets.union( > > slavesToOffers.keySet(), > > slavesToActiveTasks.keySet())); > > ```
I actually need a mutable set here to support slave removal. > On April 15, 2015, 7:58 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java, > > line 185 > > <https://reviews.apache.org/r/32597/diff/4/?file=930944#file930944line185> > > > > Did you consider using a `Multiset` instead? > > > > `ImmutableMultiset.copyOf(FluentIterable...);` That's a great suggestion! Reduced some complexity in task filtering. > On April 15, 2015, 7:58 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java, > > line 198 > > <https://reviews.apache.org/r/32597/diff/4/?file=930944#file930944line198> > > > > Either here or at the call site, please call out the reason we arrange > > elements this way. Done. > On April 15, 2015, 7:58 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java, > > line 203 > > <https://reviews.apache.org/r/32597/diff/4/?file=930944#file930944line203> > > > > `splice` could be semantically confused with the javascript function: > > http://www.w3schools.com/jsref/jsref_splice.asp > > > > Consider a name specific to the use case, like `getPreemptionSequence`. Done. > On April 15, 2015, 7:58 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlot.java, > > line 23 > > <https://reviews.apache.org/r/32597/diff/4/?file=930945#file930945line23> > > > > Since there's already diff churn - do you think `PreemptionProposal` is > > a better name? No strong preference. Changed to proposal. > On April 15, 2015, 7:58 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java, > > line 134 > > <https://reviews.apache.org/r/32597/diff/4/?file=930946#file930946line134> > > > > This is a pretty gnarly method signature. Do you think there's > > opportunity to generalize things so you can hide the distinction between > > `Optional<HostOffer>` and `Iterable<PreemptionVictim>`? Well, it may make signature a bit tidier but I think we will certainly lose on readability. Given the multitude of types involved here I'd rather prefer keeping things concrete. - Maxim ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32597/#review80256 ----------------------------------------------------------- On April 15, 2015, 7:05 p.m., Maxim Khutornenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/32597/ > ----------------------------------------------------------- > > (Updated April 15, 2015, 7:05 p.m.) > > > Review request for Aurora, Bill Farner and Zameer Manji. > > > Bugs: AURORA-1219 > https://issues.apache.org/jira/browse/AURORA-1219 > > > Repository: aurora > > > Description > ------- > > This is addressing the preemption efficiency loss due to making preemptor > async. Summary: > - Implemented fair task processing by evaluating pending task groups in > round-robin fashion (e.g.: {G1:3, G2:2} -> {G1, G2, G1, G2, G1}) against all > available slaves. > - Moved relevant functionality from PreemptionSlotFinder (now > PreemptionVictimFilter) into PendingTaskProcessor. > > The bulk of new functionality is in PendingTaskIterator and > PendingTaskProcessor. The rest is refactoring to adjust to the new traversal > approach. > > > Diffs > ----- > > > src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java > 00919b7910704c5025465e1071378a978e5e60a3 > > src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlot.java > PRE-CREATION > > src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java > f16f964f56f0f9da523950891293083f1bd86780 > src/main/java/org/apache/aurora/scheduler/async/preemptor/Preemptor.java > 5200811ec8fe6fedf42ae2712f29051a8c0bf4a9 > > src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorMetrics.java > dc7eb4421ff305dca32f36c83605c2864fea8b11 > > src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModule.java > 7cea881a8c3c11142bd04b3c794cd86a310b15e7 > src/main/java/org/apache/aurora/scheduler/base/TaskGroupKey.java > 6af3949b85297043640edccc1a490906c0fcff6c > src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java > 7bb1e7a9f27088636d6549c089b1d079dfeaf2ee > > src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessorTest.java > 8a9a3b7d9686e29632f4e267f591cdb19826e0e7 > > src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImplTest.java > 64283fab8c61b841007d7c0a02b083b3067bc78d > > src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorSlotFinderTest.java > eed2de99a145dd2124b7f2b4d401214f1d8adf2e > > Diff: https://reviews.apache.org/r/32597/diff/ > > > Testing > ------- > > ./gradlew -Pq build > Manual testing in vagrant > > > Thanks, > > Maxim Khutornenko > >