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 32597: Improving async preemptor efficiency.
Date Tue, 21 Apr 2015 01:08:17 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message