> On April 17, 2015, 5:59 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java, > > lines 142-144 > > <https://reviews.apache.org/r/32597/diff/5/?file=931268#file931268line142> > > > > ``` > > Set<String> allSlaves = Sets.newHashSet(Iterables.concat( > > slavesToOffers.keySet(), > > slavesToActiveTasks.keySet())); > > ``` > > Maxim Khutornenko wrote: > Not opposed to the change but how is this better exactly? The way it's > currently written will iterate over all available slaves only once whereas > the proposed change will have to do it twice (at least the way I read it).
Are you assuming that `Iterables.concat` eagerly iterates over the inputs? If so, that is false: http://docs.guava-libraries.googlecode.com/git-history/release/javadoc/com/google/common/collect/Iterables.html#concat(java.lang.Iterable...) I prefer the snippet i've posted since it uses a single statement for a single logical entity. Of course, if that has the side-effect of signifcantly worse performance, it is not worth it. > On April 17, 2015, 5:59 p.m., Bill Farner wrote: > > src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessorTest.java, > > line 96 > > <https://reviews.apache.org/r/32597/diff/5/?file=931276#file931276line96> > > > > Should have brought this up in a previous review, but i'm uncomfortable > > with using a mock to control the behavior of a data structure. It really > > feels like the test knows way too much about the internals of the class at > > this point, given that this is internally-managed state. Is there any > > particular reason we shouldn't use a concrete `BiCache` instance here? > > Maxim Khutornenko wrote: > Refactored to use concrete instance. Thanks!! > On April 17, 2015, 5:59 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java, > > line 184 > > <https://reviews.apache.org/r/32597/diff/5/?file=931268#file931268line184> > > > > It's probably not worth changing code, but might be worth noting that > > this breaks round-robin, since the position is reset. > > Maxim Khutornenko wrote: > Not sure what you mean. The whole reason to have a consuming iterator > here is to ensure the position is not reset when unsatisfiable groups are > removed. Aha, thanks - it slipped my mind that the consuming iterator and removeAll were affecting the same data. You're right. - Bill ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32597/#review80480 ----------------------------------------------------------- On April 21, 2015, 1:12 a.m., Maxim Khutornenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/32597/ > ----------------------------------------------------------- > > (Updated April 21, 2015, 1:12 a.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/PreemptionProposal.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 > >
