Re: Review Request 32597: Improving async preemptor efficiency.

[Aurora ReviewBot]

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

Ship it!


Master (d10d2d1) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


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

Re: Review Request 32597: Improving async preemptor efficiency.

[Maxim Khutornenko]

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

(Updated April 21, 2015, 9:15 p.m.)


Review request for Aurora, Bill Farner and Zameer Manji.


Changes
---

Bill's comments.


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 (updated)
-

  
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

Re: Review Request 32597: Improving async preemptor efficiency.

[Maxim Khutornenko]

> 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
> > 
> >
> > ```
> > Set 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).
> 
> Bill Farner wrote:
> 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.

Benchmarked both approaches and could not find any difference in perf. Taking 
your suggestion.


- Maxim


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

Re: Review Request 32597: Improving async preemptor efficiency.

[Bill Farner]

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

Ship it!


LGTM overall, but i'd like to converge on the `Sets.newHashSet` usage before 
this lands.

- Bill Farner


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

Re: Review Request 32597: Improving async preemptor efficiency.

[Bill Farner]

> 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
> > 
> >
> > ```
> > Set 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
> > 
> >
> > 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
> > 
> >
> > 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/tes

Re: Review Request 32597: Improving async preemptor efficiency.

[Aurora ReviewBot]

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

Ship it!


Master (4b90339) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


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

Re: Review Request 32597: Improving async preemptor efficiency.

[Maxim Khutornenko]

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


Changes
---

Bill's comments.


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 (updated)
-

  
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

Re: Review Request 32597: Improving async preemptor efficiency.

[Maxim Khutornenko]

> On April 16, 2015, 11:08 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java,
> >  line 183
> > 
> >
> > Don't we want `pendingGroups.remove(group)`?

This will only remove a single occurence while we want to delete all of them.


- Maxim


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


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

Re: Review Request 32597: Improving async preemptor efficiency.

[Maxim Khutornenko]

> 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
> > 
> >
> > ```
> > Set 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
> > 
> >
> > "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
> > 
> >
> > "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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > Less efficient when there are many task groups, but i find this easier 
> > to read:
> > 
> > ```
> > List instructions = Lists.newLinkedList();
> > Set 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
> > 
> >
> > 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
> > 
> >
> > `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
> > 
> >
> > `@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
> > 
> >
> > 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
> > 
> >
> > 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:
> 

Re: Review Request 32597: Improving async preemptor efficiency.

[Bill Farner]

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



src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java


```
Set allSlaves = Sets.newHashSet(Iterables.concat(
slavesToOffers.keySet(),
slavesToActiveTasks.keySet()));
```



src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java


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



src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java


"neither of the slaves"  what does that mean?



src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java


It's probably not worth changing code, but might be worth noting that this 
breaks round-robin, since the position is reset.



src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java


Given that this is all private/local code, consider 
`HashMultiset.create(Iterable)` here and avoid the immutable copy -> mutable 
copy dance.



src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java


Less efficient when there are many task groups, but i find this easier to 
read:

```
List instructions = Lists.newLinkedList();
Set 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);
}
  }
}
```



src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionProposal.java


a small doc would be nice here



src/main/java/org/apache/aurora/scheduler/async/preemptor/Preemptor.java


`slaveId`


https://google-styleguide.googlecode.com/svn/trunk/javaguide.html#s5.3-camel-case



src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessorTest.java


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?



src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessorTest.java


`@Nullable String slaveId`



src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessorTest.java


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.



src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImplTest.java


I don't think anyObject() is what you want, that's for matching arguments.  
This returns `null`.



src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImplTest.java


Related to the `OfferManager.andReturn` mock above, you could return a 
constant there, and expect the same constant here.  Then you get to drop all 
the `eq()`s.


- Bill Farner


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, 

Re: Review Request 32597: Improving async preemptor efficiency.

[Zameer Manji]

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

Ship it!



src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java


Don't we want `pendingGroups.remove(group)`?


- Zameer Manji


On April 15, 2015, 6:39 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, 6:39 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/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
> 
>

Re: Review Request 32597: Improving async preemptor efficiency.

[Aurora ReviewBot]

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

Ship it!


Master (b18dc44) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


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

Re: Review Request 32597: Improving async preemptor efficiency.

[Maxim Khutornenko]

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


Changes
---

Bill's comments.


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 (updated)
-

  
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

Re: Review Request 32597: Improving async preemptor efficiency.

[Maxim Khutornenko]

> On April 15, 2015, 7:58 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java,
> >  line 140
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > `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
> > 
> >
> > 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
> > 
> >
> > This is a pretty gnarly method signature.  Do you think there's 
> > opportunity to generalize things so you can hide the distinction between 
> > `Optional` and `Iterable`?

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

Re: Review Request 32597: Improving async preemptor efficiency.

[Bill Farner]

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



src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java


How about
```
ImmutableSet.copyOf(Sets.union(
slavesToOffers.keySet(),
slavesToActiveTasks.keySet()));
```



src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java


This is a point where a higher-level comment explaining the algorithm would 
be really helpful.  Up to this point, you're gathering requisite state, but now 
you're clearly making a conscious decision with the consumingIterator.  If you 
call out some of the goals of the algorithm before this, it will make the 
implementation decisions more obvious.

To be specific - you should call out things like each preemption round will 
place at most one task per slave.



src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java


Did you consider using a `Multiset` instead?

`ImmutableMultiset.copyOf(FluentIterable...);`



src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java


Either here or at the call site, please call out the reason we arrange 
elements this way.



src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java


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



src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlot.java


Since there's already diff churn - do you think `PreemptionProposal` is a 
better name?



src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java


This is a pretty gnarly method signature.  Do you think there's opportunity 
to generalize things so you can hide the distinction between 
`Optional` and `Iterable`?


- Bill Farner


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 

Re: Review Request 32597: Improving async preemptor efficiency.

[Aurora ReviewBot]

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

Ship it!


Master (10e75fc) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


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

Re: Review Request 32597: Improving async preemptor efficiency.

[Maxim Khutornenko]

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


Changes
---

Reworked algorithm to take advantage of generalized TaskGroup pending pool and 
get rid of circular buffer.


Bugs: AURORA-1219
https://issues.apache.org/jira/browse/AURORA-1219


Repository: aurora


Description (updated)
---

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 (updated)
-

  
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

Re: Review Request 32597: Improving async preemptor efficiency.

[Zameer Manji]

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

Ship it!


Inverting the loop is a great idea.


src/main/java/org/apache/aurora/scheduler/base/TaskGroupKey.java


A ticket should be filed to ensure we do not lose track of this improvement.


- Zameer Manji


On April 3, 2015, 1:04 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32597/
> ---
> 
> (Updated April 3, 2015, 1:04 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 tasks in the order 
> of their TaskGroupKeys (largest count first)
> - Inverted the traversal of pending tasks to better reuse loop invariants. 
> Every slave is sized up against all pending tasks until a match is found
> - 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/PendingTaskIterator.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java
>  67ad5d7f9909bc892301c19586561b6cdbfe79e6 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlot.java 
> PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotCache.java
>  b5a42a0a0498f06a382576e2c3a839bbb7f1d2b1 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java
>  427c0de205c28540b217e817bdbab10b4af5aee4 
>   src/main/java/org/apache/aurora/scheduler/async/preemptor/Preemptor.java 
> 77617ec9a2bcd062d5b80cd2b4c58dc80514 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModule.java
>  1092c055588363794b37a965fb2f17a6e50d92d7 
>   src/main/java/org/apache/aurora/scheduler/base/TaskGroupKey.java 
> 6af3949b85297043640edccc1a490906c0fcff6c 
>   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
> d212bbdf3c37be8e1e00eb169cf40b90fe69ed5f 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskIteratorTest.java
>  PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessorTest.java
>  bcd1b4e854f5ea227268c73156bc97c7c937c1de 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotCacheTest.java
>  80bd13a192bda64759b5a93749ec47ddfeae504a 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImplTest.java
>  281f4e02650727aa5d0a35a09dcf0eb823ad1b50 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorSlotFinderTest.java
>  b80e558f18b817814e4768b13ff94aa816d28543 
> 
> Diff: https://reviews.apache.org/r/32597/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> Manual testing in vagrant
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 32597: Improving async preemptor efficiency.

[Zameer Manji]

> On April 2, 2015, 8:37 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskIterator.java,
> >  line 68
> > 
> >
> > What's the motivation behind the ordering?  It also seems somewhat 
> > contradictory along with the class doc "fair evaluation order of all task 
> > groups regardless of their task count".
> 
> Maxim Khutornenko wrote:
> The idea was to give larger jobs a bigger chance to succeed. E.g. in case 
> tasks from different groups equally qualify for a slave, a task from a larger 
> job wins. I am not entirely sold on that idea though. Perhaps we can drop 
> sorting alltogether for simplicity?

+1 I think we should also drop this sorting for simplicity. I also don't think 
we prefer large jobs anywhere else in our scheduling so we should not prefer 
them here as well/


- Zameer


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


On April 3, 2015, 1:04 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32597/
> ---
> 
> (Updated April 3, 2015, 1:04 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 tasks in the order 
> of their TaskGroupKeys (largest count first)
> - Inverted the traversal of pending tasks to better reuse loop invariants. 
> Every slave is sized up against all pending tasks until a match is found
> - 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/PendingTaskIterator.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java
>  67ad5d7f9909bc892301c19586561b6cdbfe79e6 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlot.java 
> PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotCache.java
>  b5a42a0a0498f06a382576e2c3a839bbb7f1d2b1 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java
>  427c0de205c28540b217e817bdbab10b4af5aee4 
>   src/main/java/org/apache/aurora/scheduler/async/preemptor/Preemptor.java 
> 77617ec9a2bcd062d5b80cd2b4c58dc80514 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModule.java
>  1092c055588363794b37a965fb2f17a6e50d92d7 
>   src/main/java/org/apache/aurora/scheduler/base/TaskGroupKey.java 
> 6af3949b85297043640edccc1a490906c0fcff6c 
>   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
> d212bbdf3c37be8e1e00eb169cf40b90fe69ed5f 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskIteratorTest.java
>  PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessorTest.java
>  bcd1b4e854f5ea227268c73156bc97c7c937c1de 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotCacheTest.java
>  80bd13a192bda64759b5a93749ec47ddfeae504a 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImplTest.java
>  281f4e02650727aa5d0a35a09dcf0eb823ad1b50 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorSlotFinderTest.java
>  b80e558f18b817814e4768b13ff94aa816d28543 
> 
> Diff: https://reviews.apache.org/r/32597/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> Manual testing in vagrant
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 32597: Improving async preemptor efficiency.

[Aurora ReviewBot]

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

Ship it!


Master (9e46dd8) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On April 3, 2015, 8:04 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32597/
> ---
> 
> (Updated April 3, 2015, 8:04 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 tasks in the order 
> of their TaskGroupKeys (largest count first)
> - Inverted the traversal of pending tasks to better reuse loop invariants. 
> Every slave is sized up against all pending tasks until a match is found
> - 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/PendingTaskIterator.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java
>  67ad5d7f9909bc892301c19586561b6cdbfe79e6 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlot.java 
> PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotCache.java
>  b5a42a0a0498f06a382576e2c3a839bbb7f1d2b1 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java
>  427c0de205c28540b217e817bdbab10b4af5aee4 
>   src/main/java/org/apache/aurora/scheduler/async/preemptor/Preemptor.java 
> 77617ec9a2bcd062d5b80cd2b4c58dc80514 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModule.java
>  1092c055588363794b37a965fb2f17a6e50d92d7 
>   src/main/java/org/apache/aurora/scheduler/base/TaskGroupKey.java 
> 6af3949b85297043640edccc1a490906c0fcff6c 
>   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
> d212bbdf3c37be8e1e00eb169cf40b90fe69ed5f 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskIteratorTest.java
>  PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessorTest.java
>  bcd1b4e854f5ea227268c73156bc97c7c937c1de 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotCacheTest.java
>  80bd13a192bda64759b5a93749ec47ddfeae504a 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImplTest.java
>  281f4e02650727aa5d0a35a09dcf0eb823ad1b50 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorSlotFinderTest.java
>  b80e558f18b817814e4768b13ff94aa816d28543 
> 
> Diff: https://reviews.apache.org/r/32597/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> Manual testing in vagrant
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 32597: Improving async preemptor efficiency.

[Maxim Khutornenko]

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

(Updated April 3, 2015, 8:04 p.m.)


Review request for Aurora, Bill Farner and Zameer Manji.


Changes
---

Bill's comments.


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 tasks in the order of 
their TaskGroupKeys (largest count first)
- Inverted the traversal of pending tasks to better reuse loop invariants. 
Every slave is sized up against all pending tasks until a match is found
- 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 (updated)
-

  
src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskIterator.java
 PRE-CREATION 
  
src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java
 67ad5d7f9909bc892301c19586561b6cdbfe79e6 
  src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlot.java 
PRE-CREATION 
  
src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotCache.java
 b5a42a0a0498f06a382576e2c3a839bbb7f1d2b1 
  
src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java
 427c0de205c28540b217e817bdbab10b4af5aee4 
  src/main/java/org/apache/aurora/scheduler/async/preemptor/Preemptor.java 
77617ec9a2bcd062d5b80cd2b4c58dc80514 
  
src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModule.java 
1092c055588363794b37a965fb2f17a6e50d92d7 
  src/main/java/org/apache/aurora/scheduler/base/TaskGroupKey.java 
6af3949b85297043640edccc1a490906c0fcff6c 
  src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
d212bbdf3c37be8e1e00eb169cf40b90fe69ed5f 
  
src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskIteratorTest.java
 PRE-CREATION 
  
src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessorTest.java
 bcd1b4e854f5ea227268c73156bc97c7c937c1de 
  
src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotCacheTest.java
 80bd13a192bda64759b5a93749ec47ddfeae504a 
  
src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImplTest.java
 281f4e02650727aa5d0a35a09dcf0eb823ad1b50 
  
src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorSlotFinderTest.java
 b80e558f18b817814e4768b13ff94aa816d28543 

Diff: https://reviews.apache.org/r/32597/diff/


Testing
---

./gradlew -Pq build
Manual testing in vagrant


Thanks,

Maxim Khutornenko

Re: Review Request 32597: Improving async preemptor efficiency.

[Maxim Khutornenko]

> On April 3, 2015, 3:37 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskIterator.java,
> >  line 39
> > 
> >
> > `{@link TaskGroupKey}`

Done.


> On April 3, 2015, 3:37 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskIterator.java,
> >  lines 44-51
> > 
> >
> > Javadoc does not preserve this formatting.  You'll want to wrap in a 
> > `{@code xxx}`
> > 
> > or ``.

Done.


> On April 3, 2015, 3:37 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskIterator.java,
> >  line 68
> > 
> >
> > What's the motivation behind the ordering?  It also seems somewhat 
> > contradictory along with the class doc "fair evaluation order of all task 
> > groups regardless of their task count".

The idea was to give larger jobs a bigger chance to succeed. E.g. in case tasks 
from different groups equally qualify for a slave, a task from a larger job 
wins. I am not entirely sold on that idea though. Perhaps we can drop sorting 
alltogether for simplicity?


> On April 3, 2015, 3:37 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskIterator.java,
> >  line 90
> > 
> >
> > s/java.lang//, here and elsewhere

Done.


> On April 3, 2015, 3:37 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java,
> >  line 138
> > 
> >
> > I can't think of a downside if the outer loop were over pending tasks 
> > rather than slaves.  The upside would be that we could avoid the 
> > `AttributeAggregate` cachine added in this diff.
> > 
> > Is there something i'm not thinking about?
> > 
> > *note* If you act on this comment, the `Iterable` comment is no longer 
> > relevant.

This approach simlifies the algorithm and imporoves efficiency as the outer 
loop is traversed only once. If we are to switch loops we would have to 
maintain some kind of circular buffer for both loops to avoid restarting slave 
iteration from head when a task/slave match is detected. 

Also, there will be extra lookup required to avoid repeatedly matching a slave 
that has already yielded a slot in previous iterations.


> On April 3, 2015, 3:37 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java,
> >  lines 144-150
> > 
> >
> > Could this be simplified by making `PeekingTaskIterator` an `Iterable`, 
> > and have each `Iterator` pass through the groups exactly once?  i.e. 
> > perhaps a circular buffer isn't the right model here, since you're having 
> > to work around it.
> > 
> > This would also allow the code here to be ignorant of `TaskGroupKey`, 
> > since you could support `Iterator#remove()`.

The main reason for this is fairness. If we always restart from head, some tail 
groups may starve. The solution is to maintain iteration state within a 
circular buffer to give all groups a similar share.


- Maxim


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


On April 2, 2015, 11:39 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32597/
> ---
> 
> (Updated April 2, 2015, 11:39 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 tasks in the order 
> of their TaskGroupKeys (largest count first)
> - Inverted the traversal of pending tasks to better reuse loop invariants. 
> Every slave is sized up against all pending tasks until a match is found
> - 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/p

Re: Review Request 32597: Improving async preemptor efficiency.

[Bill Farner]

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



src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskIterator.java


`{@link TaskGroupKey}`



src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskIterator.java


Javadoc does not preserve this formatting.  You'll want to wrap in a 
`{@code xxx}`

or ``.



src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskIterator.java


What's the motivation behind the ordering?  It also seems somewhat 
contradictory along with the class doc "fair evaluation order of all task 
groups regardless of their task count".



src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskIterator.java


s/java.lang//, here and elsewhere



src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java


I can't think of a downside if the outer loop were over pending tasks 
rather than slaves.  The upside would be that we could avoid the 
`AttributeAggregate` cachine added in this diff.

Is there something i'm not thinking about?

*note* If you act on this comment, the `Iterable` comment is no longer 
relevant.



src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java


Could this be simplified by making `PeekingTaskIterator` an `Iterable`, and 
have each `Iterator` pass through the groups exactly once?  i.e. perhaps a 
circular buffer isn't the right model here, since you're having to work around 
it.

This would also allow the code here to be ignorant of `TaskGroupKey`, since 
you could support `Iterator#remove()`.


- Bill Farner


On April 2, 2015, 11:39 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32597/
> ---
> 
> (Updated April 2, 2015, 11:39 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 tasks in the order 
> of their TaskGroupKeys (largest count first)
> - Inverted the traversal of pending tasks to better reuse loop invariants. 
> Every slave is sized up against all pending tasks until a match is found
> - 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/PendingTaskIterator.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java
>  67ad5d7f9909bc892301c19586561b6cdbfe79e6 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlot.java 
> PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotCache.java
>  b5a42a0a0498f06a382576e2c3a839bbb7f1d2b1 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java
>  427c0de205c28540b217e817bdbab10b4af5aee4 
>   src/main/java/org/apache/aurora/scheduler/async/preemptor/Preemptor.java 
> 77617ec9a2bcd062d5b80cd2b4c58dc80514 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModule.java
>  1092c055588363794b37a965fb2f17a6e50d92d7 
>   src/main/java/org/apache/aurora/scheduler/base/TaskGroupKey.java 
> 6af3949b85297043640edccc1a490906c0fcff6c 
>   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
> d212bbdf3c37be8e1e00eb169cf40b90fe69ed5f 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskIteratorTest.java
>  PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessorTest.java
>  bcd1b4e854f5ea227268c73156bc97c7c937c1de 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotCacheTest.java
>  80bd13a192bda64759b5a93749ec47ddfeae504a 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImplTest.java
>  281f4e02650727aa5d0a35a09dcf0eb823ad1b50 
>   
> src/test/java/org/apache/aurora

Re: Review Request 32597: Improving async preemptor efficiency.

[Aurora ReviewBot]

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


Master (a7b95d9) is red with this patch.
  ./build-support/jenkins/build.sh

:check
:build
:api:assemble
:api:compileTestJava UP-TO-DATE
:api:processTestResources UP-TO-DATE
:api:testClasses UP-TO-DATE
:api:test UP-TO-DATE
:api:check UP-TO-DATE
:api:build
:buildSrc:compileJava UP-TO-DATE
:buildSrc:processResources UP-TO-DATE
:buildSrc:classes UP-TO-DATE
:buildSrc:jar
:buildSrc:assemble
:buildSrc:compileTestJava UP-TO-DATE
:buildSrc:processTestResources UP-TO-DATE
:buildSrc:testClasses UP-TO-DATE
:buildSrc:test UP-TO-DATE
:buildSrc:check UP-TO-DATE
:buildSrc:build

BUILD SUCCESSFUL

Total time: 4 mins 1.746 secs
+ export PIP_DEFAULT_TIMEOUT=60
+ PIP_DEFAULT_TIMEOUT=60
+ mkdir -p third_party
+ pip install -d third_party -r /dev/fd/63
++ grep -v mesos.native 3rdparty/python/requirements.txt
Downloading/unpacking bottle==0.11.6 (from -r /dev/fd/63 (line 1))
  Saved ./third_party/bottle-0.11.6-py2.py3-none-any.whl
Downloading/unpacking CherryPy==3.6.0 (from -r /dev/fd/63 (line 2))

pip can't proceed with requirement 'CherryPy==3.6.0 (from -r /dev/fd/63 (line 
2))' due to a pre-existing build directory.
 location: /tmp/user/2395/pip_build_jenkins/CherryPy
This is likely due to a previous installation that failed.
pip is being responsible and not assuming it can delete this.
Please delete it and try again.

Storing debug log for failure in /home/jenkins/.pip/pip.log


I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On April 2, 2015, 11:39 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32597/
> ---
> 
> (Updated April 2, 2015, 11:39 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 tasks in the order 
> of their TaskGroupKeys (largest count first)
> - Inverted the traversal of pending tasks to better reuse loop invariants. 
> Every slave is sized up against all pending tasks until a match is found
> - 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/PendingTaskIterator.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java
>  67ad5d7f9909bc892301c19586561b6cdbfe79e6 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlot.java 
> PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotCache.java
>  b5a42a0a0498f06a382576e2c3a839bbb7f1d2b1 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java
>  427c0de205c28540b217e817bdbab10b4af5aee4 
>   src/main/java/org/apache/aurora/scheduler/async/preemptor/Preemptor.java 
> 77617ec9a2bcd062d5b80cd2b4c58dc80514 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModule.java
>  1092c055588363794b37a965fb2f17a6e50d92d7 
>   src/main/java/org/apache/aurora/scheduler/base/TaskGroupKey.java 
> 6af3949b85297043640edccc1a490906c0fcff6c 
>   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
> d212bbdf3c37be8e1e00eb169cf40b90fe69ed5f 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskIteratorTest.java
>  PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessorTest.java
>  bcd1b4e854f5ea227268c73156bc97c7c937c1de 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotCacheTest.java
>  80bd13a192bda64759b5a93749ec47ddfeae504a 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImplTest.java
>  281f4e02650727aa5d0a35a09dcf0eb823ad1b50 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorSlotFinderTest.java
>  b80e558f18b817814e4768b13ff94aa816d28543 
> 
> Diff: https://reviews.apache.org/r/32597/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> Manual testing in vagrant
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 32597: Improving async preemptor efficiency.

[Maxim Khutornenko]

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

(Updated April 2, 2015, 11:39 p.m.)


Review request for Aurora, Bill Farner and Zameer Manji.


Changes
---

Rebased.


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 tasks in the order of 
their TaskGroupKeys (largest count first)
- Inverted the traversal of pending tasks to better reuse loop invariants. 
Every slave is sized up against all pending tasks until a match is found
- 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.

Will not apply cleanly. Diffed against https://reviews.apache.org/r/32352/


Diffs (updated)
-

  
src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskIterator.java
 PRE-CREATION 
  
src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java
 67ad5d7f9909bc892301c19586561b6cdbfe79e6 
  src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlot.java 
PRE-CREATION 
  
src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotCache.java
 b5a42a0a0498f06a382576e2c3a839bbb7f1d2b1 
  
src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java
 427c0de205c28540b217e817bdbab10b4af5aee4 
  src/main/java/org/apache/aurora/scheduler/async/preemptor/Preemptor.java 
77617ec9a2bcd062d5b80cd2b4c58dc80514 
  
src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModule.java 
1092c055588363794b37a965fb2f17a6e50d92d7 
  src/main/java/org/apache/aurora/scheduler/base/TaskGroupKey.java 
6af3949b85297043640edccc1a490906c0fcff6c 
  src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
d212bbdf3c37be8e1e00eb169cf40b90fe69ed5f 
  
src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskIteratorTest.java
 PRE-CREATION 
  
src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessorTest.java
 bcd1b4e854f5ea227268c73156bc97c7c937c1de 
  
src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotCacheTest.java
 80bd13a192bda64759b5a93749ec47ddfeae504a 
  
src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImplTest.java
 281f4e02650727aa5d0a35a09dcf0eb823ad1b50 
  
src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorSlotFinderTest.java
 b80e558f18b817814e4768b13ff94aa816d28543 

Diff: https://reviews.apache.org/r/32597/diff/


Testing
---

./gradlew -Pq build
Manual testing in vagrant


Thanks,

Maxim Khutornenko

Re: Review Request 32597: Improving async preemptor efficiency.

[Maxim Khutornenko]

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

(Updated April 2, 2015, 11:39 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 (updated)
---

This is addressing the preemption efficiency loss due to making preemptor 
async. Summary:
- Implemented fair task processing by evaluating pending tasks in the order of 
their TaskGroupKeys (largest count first)
- Inverted the traversal of pending tasks to better reuse loop invariants. 
Every slave is sized up against all pending tasks until a match is found
- 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/PendingTaskIterator.java
 PRE-CREATION 
  
src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java
 67ad5d7f9909bc892301c19586561b6cdbfe79e6 
  src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlot.java 
PRE-CREATION 
  
src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotCache.java
 b5a42a0a0498f06a382576e2c3a839bbb7f1d2b1 
  
src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java
 427c0de205c28540b217e817bdbab10b4af5aee4 
  src/main/java/org/apache/aurora/scheduler/async/preemptor/Preemptor.java 
77617ec9a2bcd062d5b80cd2b4c58dc80514 
  
src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModule.java 
1092c055588363794b37a965fb2f17a6e50d92d7 
  src/main/java/org/apache/aurora/scheduler/base/TaskGroupKey.java 
6af3949b85297043640edccc1a490906c0fcff6c 
  src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
d212bbdf3c37be8e1e00eb169cf40b90fe69ed5f 
  
src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskIteratorTest.java
 PRE-CREATION 
  
src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessorTest.java
 bcd1b4e854f5ea227268c73156bc97c7c937c1de 
  
src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotCacheTest.java
 80bd13a192bda64759b5a93749ec47ddfeae504a 
  
src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImplTest.java
 281f4e02650727aa5d0a35a09dcf0eb823ad1b50 
  
src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorSlotFinderTest.java
 b80e558f18b817814e4768b13ff94aa816d28543 

Diff: https://reviews.apache.org/r/32597/diff/


Testing
---

./gradlew -Pq build
Manual testing in vagrant


Thanks,

Maxim Khutornenko

Re: Review Request 32597: Improving async preemptor efficiency.

[Aurora ReviewBot]

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


This patch does not apply cleanly on master (3ab8a9e), do you need to rebase?

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On March 28, 2015, 3:32 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32597/
> ---
> 
> (Updated March 28, 2015, 3:32 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 tasks in the order 
> of their TaskGroupKeys (largest count first)
> - Inverted the traversal of pending tasks to better reuse loop invariants. 
> Every slave is sized up against all pending tasks until a match is found
> - 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.
> 
> Will not apply cleanly. Diffed against https://reviews.apache.org/r/32352/
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskIterator.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlot.java 
> PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotCache.java
>  4ca36e5fe2cfd326f4d4f37f70dbcd0060109e73 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java
>  84bcdc57d798aca229d4f184cae065ec4dcf8fc5 
>   src/main/java/org/apache/aurora/scheduler/async/preemptor/Preemptor.java 
> 84791a272f245c729706af95d70c7f1631bfe99c 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModule.java
>  7034a07eae1f94d7a0bbccdf8146cf3ed0a5424e 
>   src/main/java/org/apache/aurora/scheduler/base/TaskGroupKey.java 
> 6af3949b85297043640edccc1a490906c0fcff6c 
>   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
> d212bbdf3c37be8e1e00eb169cf40b90fe69ed5f 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskIteratorTest.java
>  PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessorTest.java
>  PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotCacheTest.java
>  80bd13a192bda64759b5a93749ec47ddfeae504a 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImplTest.java
>  d17c4fb513afdb7d8ef6d7c2b0aef86c1f47c082 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorSlotFinderTest.java
>  b80e558f18b817814e4768b13ff94aa816d28543 
> 
> Diff: https://reviews.apache.org/r/32597/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> Manual testing in vagrant
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Review Request 32597: Improving async preemptor efficiency.

[Maxim Khutornenko]

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

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 tasks in the order of 
their TaskGroupKeys (largest count first)
- Inverted the traversal of pending tasks to better reuse loop invariants. 
Every slave is sized up against all pending tasks until a match is found
- 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.

Will not apply cleanly. Diffed against https://reviews.apache.org/r/32352/


Diffs
-

  
src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskIterator.java
 PRE-CREATION 
  
src/main/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessor.java
 PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlot.java 
PRE-CREATION 
  
src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotCache.java
 4ca36e5fe2cfd326f4d4f37f70dbcd0060109e73 
  
src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java
 84bcdc57d798aca229d4f184cae065ec4dcf8fc5 
  src/main/java/org/apache/aurora/scheduler/async/preemptor/Preemptor.java 
84791a272f245c729706af95d70c7f1631bfe99c 
  
src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModule.java 
7034a07eae1f94d7a0bbccdf8146cf3ed0a5424e 
  src/main/java/org/apache/aurora/scheduler/base/TaskGroupKey.java 
6af3949b85297043640edccc1a490906c0fcff6c 
  src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
d212bbdf3c37be8e1e00eb169cf40b90fe69ed5f 
  
src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskIteratorTest.java
 PRE-CREATION 
  
src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessorTest.java
 PRE-CREATION 
  
src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotCacheTest.java
 80bd13a192bda64759b5a93749ec47ddfeae504a 
  
src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImplTest.java
 d17c4fb513afdb7d8ef6d7c2b0aef86c1f47c082 
  
src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorSlotFinderTest.java
 b80e558f18b817814e4768b13ff94aa816d28543 

Diff: https://reviews.apache.org/r/32597/diff/


Testing
---

./gradlew -Pq build
Manual testing in vagrant


Thanks,

Maxim Khutornenko