> 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()));
> >     ```

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


> On April 17, 2015, 5:59 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java,
> >  line 149
> > <https://reviews.apache.org/r/32597/diff/5/?file=931268#file931268line149>
> >
> >     "Reservations are made for at most one task group per slave."
> >     
> >     This reads as though you might make multiple reservations on a slave, 
> > so long as they are from different task groups.

Rephrased.


> On April 17, 2015, 5:59 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java,
> >  line 150
> > <https://reviews.apache.org/r/32597/diff/5/?file=931268#file931268line150>
> >
> >     "neither of the slaves"  what does that mean?

Rephrased.


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

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.


> On April 17, 2015, 5:59 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java,
> >  line 193
> > <https://reviews.apache.org/r/32597/diff/5/?file=931268#file931268line193>
> >
> >     Given that this is all private/local code, consider 
> > `HashMultiset.create(Iterable)` here and avoid the immutable copy -> 
> > mutable copy dance.

Makes sense. Done.


> On April 17, 2015, 5:59 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java,
> >  line 215
> > <https://reviews.apache.org/r/32597/diff/5/?file=931268#file931268line215>
> >
> >     Less efficient when there are many task groups, but i find this easier 
> > to read:
> >     
> >     ```
> >     List<TaskGroupKey> instructions = Lists.newLinkedList();
> >     Set<TaskGroupKey> keys = ImmutableSet.copyOf(groups.elementSet());
> >     while (!groups.isEmpty()) {
> >       for (TaskGroupKey key : keys) {
> >         if (groups.contains(key)) {
> >           instructions.add(key);
> >           // Removes a single instance of key.
> >           groups.remove(key);
> >         }
> >       }
> >     }
> >     ```

Sure, done.


> On April 17, 2015, 5:59 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionProposal.java,
> >  line 23
> > <https://reviews.apache.org/r/32597/diff/5/?file=931269#file931269line23>
> >
> >     a small doc would be nice here

Added.


> On April 17, 2015, 5:59 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/async/preemptor/Preemptor.java, 
> > line 92
> > <https://reviews.apache.org/r/32597/diff/5/?file=931271#file931271line92>
> >
> >     `slaveId`
> >     
> >     
> > https://google-styleguide.googlecode.com/svn/trunk/javaguide.html#s5.3-camel-case

No idea how it got like that. Changed.


> On April 17, 2015, 5:59 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessorTest.java,
> >  line 294
> > <https://reviews.apache.org/r/32597/diff/5/?file=931276#file931276line294>
> >
> >     `@Nullable String slaveId`

Added.


> On April 17, 2015, 5:59 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessorTest.java,
> >  line 298
> > <https://reviews.apache.org/r/32597/diff/5/?file=931276#file931276line298>
> >
> >     I'd like to avoid randomness in tests, as trivial as it may seem here.  
> > It's reasonable to assume that calling `makeTask()` with identical 
> > arguments should at least produce an `equals()` object.  I'd rather see a 
> > task id argument to make that true.

Changed.


> On April 17, 2015, 5:59 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImplTest.java,
> >  line 80
> > <https://reviews.apache.org/r/32597/diff/5/?file=931277#file931277line80>
> >
> >     I don't think anyObject() is what you want, that's for matching 
> > arguments.  This returns `null`.

Done.


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

Refactored to use concrete instance.


- Maxim


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


On April 16, 2015, 1:39 a.m., Maxim Khutornenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32597/
> -----------------------------------------------------------
> 
> (Updated April 16, 2015, 1:39 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
> 
>

Reply via email to