Re: Review Request 51929: Scheduling multiple tasks per round.

2016-09-20 Thread Stephan Erb 

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


Ship it!




Ship It!

- Stephan Erb


On Sept. 21, 2016, 12:02 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51929/
> ---
> 
> (Updated Sept. 21, 2016, 12:02 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Stephan Erb, and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This is phase 2 of scheduling perf improvement effort started in 
> https://reviews.apache.org/r/51759/.
> 
> We can now take multiple (configurable) number of task IDs from a given 
> `TaskGroup` per scheduling. The idea is to go deeper through the offer queue 
> and assign more than one task if possible. This approach delivers 
> substantially better MTTA and still ensures fairness across multiple 
> `TaskGroups`. We have observed almost linear improvement in MTTA (4x+ with 5 
> tasks per round), which suggest the `max_tasks_per_schedule_attempt` can be 
> set even higher if the majority of cluster jobs have large number of 
> instances and/or update batch sizes.
> 
> As far as a single round perf goes, we can consider the following 2 
> worst-case scenarios:
> - master: single task scheduling fails after trying all offers in the queue
> - this patch: N tasks launched with the very last N offers in the queue + `(N 
> x single_task_launch_latency)`
> 
> Assuming that matching N tasks against M offers takes exactly the same time 
> as 1 task against M offers (as they all share the same `TaskGroup`), the only 
> measurable difference comes from the additional `N x 
> single_task_launch_latency` overhead. Based on real cluster observations, the 
> `single_task_launch_latency` is less than 1% of a single task scheduling 
> attempt, which is << than the savings from avoided additional scheduling 
> rounds. 
> 
> As far as jmh results go, the new approach (batching + multiple tasks per 
> round) is only slightly more demanding (~8%). Both results though are MUCH 
> higher than the real cluster perf, which just confirms we are not bound by 
> CPU time here:
> 
> Master:
> ```
> Benchmark
> Mode  Cnt  Score Error  Units
> SchedulingBenchmarks.InsufficientResourcesSchedulingBenchmark.runBenchmark  
> thrpt   10  17126.183 ± 488.425  ops/s
> ```
> 
> This patch:
> ```
> Benchmark
> Mode  Cnt  Score Error  Units
> SchedulingBenchmarks.InsufficientResourcesSchedulingBenchmark.runBenchmark  
> thrpt   10  15838.051 ± 187.890  ops/s
> ```
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java 
> 6f1cbfbc4510a037cffc95fee54f62f463d2b534 
>   src/main/java/org/apache/aurora/scheduler/filter/AttributeAggregate.java 
> 87b9e1928ab2d44668df1123f32ffdc4197c0c70 
>   src/main/java/org/apache/aurora/scheduler/scheduling/SchedulingModule.java 
> 664bc6cf964ede2473a4463e58bcdbcb65bc7413 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java 
> 5d319557057e27fd5fc6d3e553e9ca9139399c50 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroups.java 
> d390c07522d22e43d79ce4370985f3643ef021ca 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskScheduler.java 
> 207d38d1ddfd373892602218a98c1daaf4a1325f 
>   src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
> 7f7b4358ef05c0f0d0e14daac1a5c25488467dc9 
>   
> src/test/java/org/apache/aurora/scheduler/events/NotifyingSchedulingFilterTest.java
>  ece476b918e6f2c128039e561eea23a94d8ed396 
>   
> src/test/java/org/apache/aurora/scheduler/filter/AttributeAggregateTest.java 
> 209f9298a1d55207b9b41159f2ab366f92c1eb70 
>   
> src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java
>  0cf23df9f373c0d9b27e55a12adefd5f5fd81ba5 
>   src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java 
> c1c3eca4a6e6c88dab6b1c69fae3e2f290b58039 
>   
> src/test/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilterTest.java
>  ee5c6528af89cc62a35fdb314358c489556d8131 
>   src/test/java/org/apache/aurora/scheduler/preemptor/PreemptorImplTest.java 
> 98048fabc00f233925b6cca015c2525980556e2b 
>   
> src/test/java/org/apache/aurora/scheduler/preemptor/PreemptorModuleTest.java 
> 2c3e5f32c774be07a5fa28c8bcf3b9a5d88059a1 
>   src/test/java/org/apache/aurora/scheduler/scheduling/TaskGroupsTest.java 
> 88729626de5fa87b45472792c59cc0ff1ade3e93 
>   
> src/test/java/org/apache/aurora/scheduler/scheduling/TaskSchedulerImplTest.java
>  a4e87d2216401f344dca64d69b945de7bcf8159a 
>

Re: Review Request 51929: Scheduling multiple tasks per round.

2016-09-20 Thread Aurora ReviewBot 

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


Ship it!




Master (8432894) 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 Sept. 20, 2016, 10:02 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51929/
> ---
> 
> (Updated Sept. 20, 2016, 10:02 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Stephan Erb, and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This is phase 2 of scheduling perf improvement effort started in 
> https://reviews.apache.org/r/51759/.
> 
> We can now take multiple (configurable) number of task IDs from a given 
> `TaskGroup` per scheduling. The idea is to go deeper through the offer queue 
> and assign more than one task if possible. This approach delivers 
> substantially better MTTA and still ensures fairness across multiple 
> `TaskGroups`. We have observed almost linear improvement in MTTA (4x+ with 5 
> tasks per round), which suggest the `max_tasks_per_schedule_attempt` can be 
> set even higher if the majority of cluster jobs have large number of 
> instances and/or update batch sizes.
> 
> As far as a single round perf goes, we can consider the following 2 
> worst-case scenarios:
> - master: single task scheduling fails after trying all offers in the queue
> - this patch: N tasks launched with the very last N offers in the queue + `(N 
> x single_task_launch_latency)`
> 
> Assuming that matching N tasks against M offers takes exactly the same time 
> as 1 task against M offers (as they all share the same `TaskGroup`), the only 
> measurable difference comes from the additional `N x 
> single_task_launch_latency` overhead. Based on real cluster observations, the 
> `single_task_launch_latency` is less than 1% of a single task scheduling 
> attempt, which is << than the savings from avoided additional scheduling 
> rounds. 
> 
> As far as jmh results go, the new approach (batching + multiple tasks per 
> round) is only slightly more demanding (~8%). Both results though are MUCH 
> higher than the real cluster perf, which just confirms we are not bound by 
> CPU time here:
> 
> Master:
> ```
> Benchmark
> Mode  Cnt  Score Error  Units
> SchedulingBenchmarks.InsufficientResourcesSchedulingBenchmark.runBenchmark  
> thrpt   10  17126.183 ± 488.425  ops/s
> ```
> 
> This patch:
> ```
> Benchmark
> Mode  Cnt  Score Error  Units
> SchedulingBenchmarks.InsufficientResourcesSchedulingBenchmark.runBenchmark  
> thrpt   10  15838.051 ± 187.890  ops/s
> ```
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java 
> 6f1cbfbc4510a037cffc95fee54f62f463d2b534 
>   src/main/java/org/apache/aurora/scheduler/filter/AttributeAggregate.java 
> 87b9e1928ab2d44668df1123f32ffdc4197c0c70 
>   src/main/java/org/apache/aurora/scheduler/scheduling/SchedulingModule.java 
> 664bc6cf964ede2473a4463e58bcdbcb65bc7413 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java 
> 5d319557057e27fd5fc6d3e553e9ca9139399c50 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroups.java 
> d390c07522d22e43d79ce4370985f3643ef021ca 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskScheduler.java 
> 207d38d1ddfd373892602218a98c1daaf4a1325f 
>   src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
> 7f7b4358ef05c0f0d0e14daac1a5c25488467dc9 
>   
> src/test/java/org/apache/aurora/scheduler/events/NotifyingSchedulingFilterTest.java
>  ece476b918e6f2c128039e561eea23a94d8ed396 
>   
> src/test/java/org/apache/aurora/scheduler/filter/AttributeAggregateTest.java 
> 209f9298a1d55207b9b41159f2ab366f92c1eb70 
>   
> src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java
>  0cf23df9f373c0d9b27e55a12adefd5f5fd81ba5 
>   src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java 
> c1c3eca4a6e6c88dab6b1c69fae3e2f290b58039 
>   
> src/test/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilterTest.java
>  ee5c6528af89cc62a35fdb314358c489556d8131 
>   src/test/java/org/apache/aurora/scheduler/preemptor/PreemptorImplTest.java 
> 98048fabc00f233925b6cca015c2525980556e2b 
>   
> src/test/java/org/apache/aurora/scheduler/preemptor/PreemptorModuleTest.java 
> 2c3e5f32c774be07a5fa28c8bcf3b9a5d88059a1 
>   src/test/java/org/apache/aurora/scheduler/scheduling/TaskGroupsTest.java 
> 88729626de5fa87b45472792c59cc0ff1ade3e93

Re: Review Request 51929: Scheduling multiple tasks per round.

2016-09-20 Thread Maxim Khutornenko 


> On Sept. 20, 2016, 6:49 p.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/scheduling/SchedulingModule.java, 
> > lines 93-96
> > 
> >
> > Regarding your notes in the RB description: I don't see a problem if we 
> > set this to a slightly higher value such as `10` or `15`. 
> > 
> > It seems like we will maintain the basic taskgroup round robin 
> > scheduling fairness even with slightly larger batch sizes, so I am ok with 
> > bumping the value.

The higher this count is the less fair do we treat lower instance count jobs in 
case the capacity of the cluster is constrained. It's hard to come up with an 
ideal default here but I feel 10-15 would be a bit too extreme for the general 
case.


> On Sept. 20, 2016, 6:49 p.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroups.java, lines 
> > 174-175
> > 
> >
> > If I understand things correctly, I believe this line could have a 
> > performance bug in it:
> > 
> > `batchWorker.execute` acquires the global storage lock before calling 
> > the `taskScheduler`. For the latter, we use the following definition:
> > 
> > ```
> > this.taskScheduler = (store, taskIds) -> {
> >   settings.rateLimiter.acquire();
> >   return taskScheduler.schedule(store, taskIds);
> > };
> > ```
> > 
> > In combination, we will be throttled by the `rateLimiter` while holding 
> > the storage lock.
> > 
> > Instead, we should try to acquire the log within the `Runnable` in 
> > `startGroup` before calling `batchWorker.execute` so that the global lock 
> > is not hold longer than absolutely necessary.

This is a valid concern. It's certainly more possible than before to hit our 
default 40 tasks per second limit now. Fixed.


- Maxim


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


On Sept. 16, 2016, 9:53 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51929/
> ---
> 
> (Updated Sept. 16, 2016, 9:53 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Stephan Erb, and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This is phase 2 of scheduling perf improvement effort started in 
> https://reviews.apache.org/r/51759/.
> 
> We can now take multiple (configurable) number of task IDs from a given 
> `TaskGroup` per scheduling. The idea is to go deeper through the offer queue 
> and assign more than one task if possible. This approach delivers 
> substantially better MTTA and still ensures fairness across multiple 
> `TaskGroups`. We have observed almost linear improvement in MTTA (4x+ with 5 
> tasks per round), which suggest the `max_tasks_per_schedule_attempt` can be 
> set even higher if the majority of cluster jobs have large number of 
> instances and/or update batch sizes.
> 
> As far as a single round perf goes, we can consider the following 2 
> worst-case scenarios:
> - master: single task scheduling fails after trying all offers in the queue
> - this patch: N tasks launched with the very last N offers in the queue + `(N 
> x single_task_launch_latency)`
> 
> Assuming that matching N tasks against M offers takes exactly the same time 
> as 1 task against M offers (as they all share the same `TaskGroup`), the only 
> measurable difference comes from the additional `N x 
> single_task_launch_latency` overhead. Based on real cluster observations, the 
> `single_task_launch_latency` is less than 1% of a single task scheduling 
> attempt, which is << than the savings from avoided additional scheduling 
> rounds. 
> 
> As far as jmh results go, the new approach (batching + multiple tasks per 
> round) is only slightly more demanding (~8%). Both results though are MUCH 
> higher than the real cluster perf, which just confirms we are not bound by 
> CPU time here:
> 
> Master:
> ```
> Benchmark
> Mode  Cnt  Score Error  Units
> SchedulingBenchmarks.InsufficientResourcesSchedulingBenchmark.runBenchmark  
> thrpt   10  17126.183 ± 488.425  ops/s
> ```
> 
> This patch:
> ```
> Benchmark
> Mode  Cnt  Score Error  Units
> SchedulingBenchmarks.InsufficientResourcesSchedulingBenchmark.runBenchmark  
> thrpt   10  15838.051 ± 187.890  ops/s
> ```
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java 
>

Re: Review Request 51929: Scheduling multiple tasks per round.

2016-09-20 Thread Maxim Khutornenko 

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

(Updated Sept. 20, 2016, 10:02 p.m.)


Review request for Aurora, Joshua Cohen, Stephan Erb, and Zameer Manji.


Changes
---

Stephan's comments.


Repository: aurora


Description
---

This is phase 2 of scheduling perf improvement effort started in 
https://reviews.apache.org/r/51759/.

We can now take multiple (configurable) number of task IDs from a given 
`TaskGroup` per scheduling. The idea is to go deeper through the offer queue 
and assign more than one task if possible. This approach delivers substantially 
better MTTA and still ensures fairness across multiple `TaskGroups`. We have 
observed almost linear improvement in MTTA (4x+ with 5 tasks per round), which 
suggest the `max_tasks_per_schedule_attempt` can be set even higher if the 
majority of cluster jobs have large number of instances and/or update batch 
sizes.

As far as a single round perf goes, we can consider the following 2 worst-case 
scenarios:
- master: single task scheduling fails after trying all offers in the queue
- this patch: N tasks launched with the very last N offers in the queue + `(N x 
single_task_launch_latency)`

Assuming that matching N tasks against M offers takes exactly the same time as 
1 task against M offers (as they all share the same `TaskGroup`), the only 
measurable difference comes from the additional `N x 
single_task_launch_latency` overhead. Based on real cluster observations, the 
`single_task_launch_latency` is less than 1% of a single task scheduling 
attempt, which is << than the savings from avoided additional scheduling 
rounds. 

As far as jmh results go, the new approach (batching + multiple tasks per 
round) is only slightly more demanding (~8%). Both results though are MUCH 
higher than the real cluster perf, which just confirms we are not bound by CPU 
time here:

Master:
```
Benchmark
Mode  Cnt  Score Error  Units
SchedulingBenchmarks.InsufficientResourcesSchedulingBenchmark.runBenchmark  
thrpt   10  17126.183 ± 488.425  ops/s
```

This patch:
```
Benchmark
Mode  Cnt  Score Error  Units
SchedulingBenchmarks.InsufficientResourcesSchedulingBenchmark.runBenchmark  
thrpt   10  15838.051 ± 187.890  ops/s
```


Diffs (updated)
-

  src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java 
6f1cbfbc4510a037cffc95fee54f62f463d2b534 
  src/main/java/org/apache/aurora/scheduler/filter/AttributeAggregate.java 
87b9e1928ab2d44668df1123f32ffdc4197c0c70 
  src/main/java/org/apache/aurora/scheduler/scheduling/SchedulingModule.java 
664bc6cf964ede2473a4463e58bcdbcb65bc7413 
  src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java 
5d319557057e27fd5fc6d3e553e9ca9139399c50 
  src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroups.java 
d390c07522d22e43d79ce4370985f3643ef021ca 
  src/main/java/org/apache/aurora/scheduler/scheduling/TaskScheduler.java 
207d38d1ddfd373892602218a98c1daaf4a1325f 
  src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
7f7b4358ef05c0f0d0e14daac1a5c25488467dc9 
  
src/test/java/org/apache/aurora/scheduler/events/NotifyingSchedulingFilterTest.java
 ece476b918e6f2c128039e561eea23a94d8ed396 
  src/test/java/org/apache/aurora/scheduler/filter/AttributeAggregateTest.java 
209f9298a1d55207b9b41159f2ab366f92c1eb70 
  
src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java 
0cf23df9f373c0d9b27e55a12adefd5f5fd81ba5 
  src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java 
c1c3eca4a6e6c88dab6b1c69fae3e2f290b58039 
  
src/test/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilterTest.java
 ee5c6528af89cc62a35fdb314358c489556d8131 
  src/test/java/org/apache/aurora/scheduler/preemptor/PreemptorImplTest.java 
98048fabc00f233925b6cca015c2525980556e2b 
  src/test/java/org/apache/aurora/scheduler/preemptor/PreemptorModuleTest.java 
2c3e5f32c774be07a5fa28c8bcf3b9a5d88059a1 
  src/test/java/org/apache/aurora/scheduler/scheduling/TaskGroupsTest.java 
88729626de5fa87b45472792c59cc0ff1ade3e93 
  
src/test/java/org/apache/aurora/scheduler/scheduling/TaskSchedulerImplTest.java 
a4e87d2216401f344dca64d69b945de7bcf8159a 
  src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 
b4d27f69ad5d4cce03da9f04424dc35d30e8af29 

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


Testing
---

All types of testing including deploying to test and production clusters.


Thanks,

Maxim Khutornenko

Re: Review Request 51929: Scheduling multiple tasks per round.

2016-09-20 Thread Maxim Khutornenko 


> On Sept. 16, 2016, 9:08 a.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroups.java, line 
> > 197
> > 
> >
> > Side show: Isn't that `if` unnecessary here and we can adjust the 
> > penality in any case? We will remove the group if `hasMore()` returns 
> > false, so any penality should be fine.
> 
> Maxim Khutornenko wrote:
> Not sure I follow. This is the place that applies penalty accrued inside 
> the `startGroup()` or removes the group if it's empty.
> 
> Stephan Erb wrote:
> Let me print some more code to make clearer what I meant.
> 
> This is the code where we compute penaltyMs depending on `group.hasMore`
> ```
> scheduledTaskPenalties.accumulate(group.getPenaltyMs());
> group.remove(scheduled);
> if (group.hasMore()) {
>   penaltyMs = firstScheduleDelay;
> }
>   }
> }
> 
> group.setPenaltyMs(penaltyMs);
> evaluateGroupLater(this, group);
> ```
> 
> 
> Later on we then drop empty groups in `evaluateGroupLater`:
> ```
>   private synchronized void evaluateGroupLater(Runnable evaluate, 
> TaskGroup group) {
> // Avoid check-then-act by holding the intrinsic lock.  If not done 
> atomically, we could
> // remove a group while a task is being added to it.
> if (group.hasMore()) {
>   executor.execute(evaluate, Amount.of(group.getPenaltyMs(), 
> Time.MILLISECONDS));
> } else {
>   groups.remove(group.getKey());
> }
>   }
> ```
> 
> What I tried to say is that we could unconditionally write `penaltyMs = 
> firstScheduleDelay;` without the `if (group.hasMore())` in the first snippet. 
> If `group.hasMore()` returns false we will remove the group anyway, so it 
> does not matter if we set a new penality or not.
> 
> This is completely unreleated to the change in this RB so feel free to 
> ignore it. It was more about checking my own understanding of the code.

I see what you meant now. The slight problem with the simplification you 
propose is that it _may_ result in a penalty where it would not happen today: 
`startGroup()` calculates the penalty even though there are no more groups left 
but the call to `evaluateGroupLater()` is delayed for some reason AND a new 
task is added into the group thus recharging it. The penalty would be applied 
the moment `evaluateGroupLater()` is finally reached. It's certainly an edge 
case but I'd prefer not changing the current behavior in the this patch.


- Maxim


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


On Sept. 16, 2016, 9:53 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51929/
> ---
> 
> (Updated Sept. 16, 2016, 9:53 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Stephan Erb, and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This is phase 2 of scheduling perf improvement effort started in 
> https://reviews.apache.org/r/51759/.
> 
> We can now take multiple (configurable) number of task IDs from a given 
> `TaskGroup` per scheduling. The idea is to go deeper through the offer queue 
> and assign more than one task if possible. This approach delivers 
> substantially better MTTA and still ensures fairness across multiple 
> `TaskGroups`. We have observed almost linear improvement in MTTA (4x+ with 5 
> tasks per round), which suggest the `max_tasks_per_schedule_attempt` can be 
> set even higher if the majority of cluster jobs have large number of 
> instances and/or update batch sizes.
> 
> As far as a single round perf goes, we can consider the following 2 
> worst-case scenarios:
> - master: single task scheduling fails after trying all offers in the queue
> - this patch: N tasks launched with the very last N offers in the queue + `(N 
> x single_task_launch_latency)`
> 
> Assuming that matching N tasks against M offers takes exactly the same time 
> as 1 task against M offers (as they all share the same `TaskGroup`), the only 
> measurable difference comes from the additional `N x 
> single_task_launch_latency` overhead. Based on real cluster observations, the 
> `single_task_launch_latency` is less than 1% of a single task scheduling 
> attempt, which is << than the savings from avoided additional scheduling 
> rounds. 
> 
> As far as jmh results go, the new approach (batching + multiple tasks per 
> round) is only slightly more demanding (~8%). Both results though are

Re: Review Request 52093: Clean up some license issues.

2016-09-20 Thread Jake Farrell 

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


Ship it!




Ship It!

- Jake Farrell


On Sept. 20, 2016, 7:48 p.m., Joshua Cohen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52093/
> ---
> 
> (Updated Sept. 20, 2016, 7:48 p.m.)
> 
> 
> Review request for Aurora and Jake Farrell.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Clean up some license issues.
> 
> 
> Diffs
> -
> 
>   LICENSE 775eaed2d116089179ef78c74631cb1ee7ac3d29 
>   NOTICE 94ea47dd39856da3b337f40bdca736293794c6f2 
>   src/main/python/apache/aurora/client/cli/cron.py 
> 33087f1a3cedd77a3ec42aec43b7b227382620b7 
>   src/test/python/apache/aurora/client/cli/test_cron.py 
> 61690d7bb53326732722577e320b8d2fe636e77f 
> 
> Diff: https://reviews.apache.org/r/52093/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joshua Cohen
> 
>

Review Request 52093: Clean up some license issues.

2016-09-20 Thread Joshua Cohen 

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

Review request for Aurora and Jake Farrell.


Repository: aurora


Description
---

Clean up some license issues.


Diffs
-

  LICENSE 775eaed2d116089179ef78c74631cb1ee7ac3d29 
  NOTICE 94ea47dd39856da3b337f40bdca736293794c6f2 
  src/main/python/apache/aurora/client/cli/cron.py 
33087f1a3cedd77a3ec42aec43b7b227382620b7 
  src/test/python/apache/aurora/client/cli/test_cron.py 
61690d7bb53326732722577e320b8d2fe636e77f 

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


Testing
---


Thanks,

Joshua Cohen

Review Request 52094: Add min_consecutive_health_checks in HealthCheckConfig

2016-09-20 Thread Kai Huang 

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

Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.


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


Repository: aurora


Description
---

Add min_consecutive_health_checks to HealthCheckConfig.

[Summary]
HealthCheckConfig should accept a new configuration value that will tell how 
many positive consecutive health checks an instance requires to move from 
STARTING to RUNNING.

[Background]
This review depends on the executor change(AURORA-1225). Please see 
https://reviews.apache.org/r/51876/ for more details and background.

[Change List]
1. Add a configuration value "min_consecutive_health_checks"(default=1) to 
HealthCheckConfig struct.
2. Modify the default value of watch_secs to be 0.
3. Add a client-side constraint: 
initial_interval_secs >= min_consecutive_health_checks * interval_secs
4. Update the unit tests for health check config in client/config.py, skip unit 
tests related to watch_secs.


Diffs
-

  src/main/python/apache/aurora/client/config.py 
0186af52f0d7d7e3981ec59bf6a01aafee2bcfb1 
  src/main/python/apache/aurora/config/schema/base.py 
845163043b0b7b2f9e7aca14677ca9f094658551 
  src/test/python/apache/aurora/client/test_config.py 
5cf68a5145ddf9478baa30453c0bcb73136fa7eb 

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


Testing
---

./build-support/jenkins/build.sh

./pants test.pytest src/test/python/apache/aurora/client::

./pants test.pytest src/test/python/apache/aurora/config::


Thanks,

Kai Huang

Re: Review Request 52074: switching from launchTask to acceptOffers

2016-09-20 Thread Mehrdad Nurolahzade 

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


Ship it!




Ship It!

- Mehrdad Nurolahzade


On Sept. 20, 2016, 12:14 p.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52074/
> ---
> 
> (Updated Sept. 20, 2016, 12:14 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Mehrdad Nurolahzade, and Zameer 
> Manji.
> 
> 
> Bugs: AURORA-1776
> https://issues.apache.org/jira/browse/AURORA-1776
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Dynamic reservation requires us to use acceptOffers so this is in preparation 
> of that work. acceptOffers is the call to dynamically reserve resources.
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeDriver.java 
> 1bfd8dbf3aec314aaeb89b756ea3a2049df8777a 
>   src/main/java/org/apache/aurora/scheduler/mesos/Driver.java 
> 14481468926b82a9bad3ee78c85f176e8ba893e2 
>   src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverService.java 
> 41b9aab55aa439b13eba3545703ec97b44690444 
>   src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 
> 925c025d21bf1d44e0c1d319f6653ecfa8899481 
>   src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java 
> a739bce1226d9435fa7d0b18e411064a4e78e49e 
> 
> Diff: https://reviews.apache.org/r/52074/diff/
> 
> 
> Testing
> ---
> 
> src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 52074: switching from launchTask to acceptOffers

2016-09-20 Thread Mehrdad Nurolahzade 

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




src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java 
(lines 50 - 54)


We should start disallowing or limiting import static. It causes poor 
readability. 

I was looking for a makeTask() method on this file.


- Mehrdad Nurolahzade


On Sept. 20, 2016, 12:14 p.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52074/
> ---
> 
> (Updated Sept. 20, 2016, 12:14 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Mehrdad Nurolahzade, and Zameer 
> Manji.
> 
> 
> Bugs: AURORA-1776
> https://issues.apache.org/jira/browse/AURORA-1776
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Dynamic reservation requires us to use acceptOffers so this is in preparation 
> of that work. acceptOffers is the call to dynamically reserve resources.
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeDriver.java 
> 1bfd8dbf3aec314aaeb89b756ea3a2049df8777a 
>   src/main/java/org/apache/aurora/scheduler/mesos/Driver.java 
> 14481468926b82a9bad3ee78c85f176e8ba893e2 
>   src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverService.java 
> 41b9aab55aa439b13eba3545703ec97b44690444 
>   src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 
> 925c025d21bf1d44e0c1d319f6653ecfa8899481 
>   src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java 
> a739bce1226d9435fa7d0b18e411064a4e78e49e 
> 
> Diff: https://reviews.apache.org/r/52074/diff/
> 
> 
> Testing
> ---
> 
> src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 52074: switching from launchTask to acceptOffers

2016-09-20 Thread Aurora ReviewBot 

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


Ship it!




Master (4745c8c) 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 Sept. 20, 2016, 7:14 p.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52074/
> ---
> 
> (Updated Sept. 20, 2016, 7:14 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Mehrdad Nurolahzade, and Zameer 
> Manji.
> 
> 
> Bugs: AURORA-1776
> https://issues.apache.org/jira/browse/AURORA-1776
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Dynamic reservation requires us to use acceptOffers so this is in preparation 
> of that work. acceptOffers is the call to dynamically reserve resources.
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeDriver.java 
> 1bfd8dbf3aec314aaeb89b756ea3a2049df8777a 
>   src/main/java/org/apache/aurora/scheduler/mesos/Driver.java 
> 14481468926b82a9bad3ee78c85f176e8ba893e2 
>   src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverService.java 
> 41b9aab55aa439b13eba3545703ec97b44690444 
>   src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 
> 925c025d21bf1d44e0c1d319f6653ecfa8899481 
>   src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java 
> a739bce1226d9435fa7d0b18e411064a4e78e49e 
> 
> Diff: https://reviews.apache.org/r/52074/diff/
> 
> 
> Testing
> ---
> 
> src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 52087: Fix host maintenance commands to properly initialize the api client.

2016-09-20 Thread Aurora ReviewBot 

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


Ship it!




Master (4745c8c) 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 Sept. 20, 2016, 6:57 p.m., Joshua Cohen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52087/
> ---
> 
> (Updated Sept. 20, 2016, 6:57 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Zameer Manji.
> 
> 
> Bugs: AURORA-1777
> https://issues.apache.org/jira/browse/AURORA-1777
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Fix host maintenance commands to properly initialize the api client.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/admin/admin.py 
> 9fc89a2842d4651ac10aa82118531c450f051712 
>   src/main/python/apache/aurora/admin/admin_util.py 
> 394deb57af9ad8832a02ceab15f33b3c1e5c902b 
>   src/main/python/apache/aurora/admin/host_maintenance.py 
> 677f870c50d1e19d34de36a3c677bb1dcd255ba0 
>   src/main/python/apache/aurora/admin/maintenance.py 
> bf446515222a1246b7fe32acd67315f8145a1212 
>   src/test/python/apache/aurora/admin/test_admin.py 
> f720742c50f774b9f5a41ba57ef251d08d79e8d1 
>   src/test/python/apache/aurora/admin/test_admin_sla.py 
> 54b5a823903b82c2082353ade132eb7b63e518db 
>   src/test/python/apache/aurora/admin/test_admin_util.py 
> f5c8c69c1109d15ee3886fb863014c3285240db1 
>   src/test/python/apache/aurora/admin/util.py 
> d0a915cd8edee6b36d671b41f43a3af4fe751ae7 
>   src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 
> e36726e0c0154e51bba40704e13eebec5c0f0d65 
> 
> Diff: https://reviews.apache.org/r/52087/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> ran e2e tests.
> 
> 
> Thanks,
> 
> Joshua Cohen
> 
>

Re: Review Request 52074: switching from launchTask to acceptOffers

2016-09-20 Thread Dmitriy Shirchenko 

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

(Updated Sept. 20, 2016, 7:14 p.m.)


Review request for Aurora, Maxim Khutornenko, Mehrdad Nurolahzade, and Zameer 
Manji.


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


Repository: aurora


Description
---

Dynamic reservation requires us to use acceptOffers so this is in preparation 
of that work. acceptOffers is the call to dynamically reserve resources.


Diffs (updated)
-

  src/jmh/java/org/apache/aurora/benchmark/fakes/FakeDriver.java 
1bfd8dbf3aec314aaeb89b756ea3a2049df8777a 
  src/main/java/org/apache/aurora/scheduler/mesos/Driver.java 
14481468926b82a9bad3ee78c85f176e8ba893e2 
  src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverService.java 
41b9aab55aa439b13eba3545703ec97b44690444 
  src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 
925c025d21bf1d44e0c1d319f6653ecfa8899481 
  src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java 
a739bce1226d9435fa7d0b18e411064a4e78e49e 

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


Testing
---

src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh


Thanks,

Dmitriy Shirchenko

Re: Review Request 52074: switching from launchTask to acceptOffers

2016-09-20 Thread Zameer Manji 

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


Fix it, then Ship it!




LGTM.

I'm surprised at how small of a ripple this change has, but I guess that's the 
power of good abstractions.


src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java 
(line 84)


All TODOs should have an "owner". For example my TODOs are marked as 
`TODO(zmanji)`. Please mark your TODO here.


- Zameer Manji


On Sept. 20, 2016, 11:08 a.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52074/
> ---
> 
> (Updated Sept. 20, 2016, 11:08 a.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Mehrdad Nurolahzade, and Zameer 
> Manji.
> 
> 
> Bugs: AURORA-1776
> https://issues.apache.org/jira/browse/AURORA-1776
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Dynamic reservation requires us to use acceptOffers so this is in preparation 
> of that work. acceptOffers is the call to dynamically reserve resources.
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeDriver.java 
> 1bfd8dbf3aec314aaeb89b756ea3a2049df8777a 
>   src/main/java/org/apache/aurora/scheduler/mesos/Driver.java 
> 14481468926b82a9bad3ee78c85f176e8ba893e2 
>   src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverService.java 
> 41b9aab55aa439b13eba3545703ec97b44690444 
>   src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 
> 925c025d21bf1d44e0c1d319f6653ecfa8899481 
>   src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java 
> a739bce1226d9435fa7d0b18e411064a4e78e49e 
> 
> Diff: https://reviews.apache.org/r/52074/diff/
> 
> 
> Testing
> ---
> 
> src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 51929: Scheduling multiple tasks per round.

2016-09-20 Thread Stephan Erb 


> On Sept. 16, 2016, 11:08 a.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroups.java, line 
> > 197
> > 
> >
> > Side show: Isn't that `if` unnecessary here and we can adjust the 
> > penality in any case? We will remove the group if `hasMore()` returns 
> > false, so any penality should be fine.
> 
> Maxim Khutornenko wrote:
> Not sure I follow. This is the place that applies penalty accrued inside 
> the `startGroup()` or removes the group if it's empty.

Let me print some more code to make clearer what I meant.

This is the code where we compute penaltyMs depending on `group.hasMore`
```
scheduledTaskPenalties.accumulate(group.getPenaltyMs());
group.remove(scheduled);
if (group.hasMore()) {
  penaltyMs = firstScheduleDelay;
}
  }
}

group.setPenaltyMs(penaltyMs);
evaluateGroupLater(this, group);
```


Later on we then drop empty groups in `evaluateGroupLater`:
```
  private synchronized void evaluateGroupLater(Runnable evaluate, TaskGroup 
group) {
// Avoid check-then-act by holding the intrinsic lock.  If not done 
atomically, we could
// remove a group while a task is being added to it.
if (group.hasMore()) {
  executor.execute(evaluate, Amount.of(group.getPenaltyMs(), 
Time.MILLISECONDS));
} else {
  groups.remove(group.getKey());
}
  }
```

What I tried to say is that we could unconditionally write `penaltyMs = 
firstScheduleDelay;` without the `if (group.hasMore())` in the first snippet. 
If `group.hasMore()` returns false we will remove the group anyway, so it does 
not matter if we set a new penality or not.

This is completely unreleated to the change in this RB so feel free to ignore 
it. It was more about checking my own understanding of the code.


> On Sept. 16, 2016, 11:08 a.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/scheduling/TaskScheduler.java, 
> > lines 208-213
> > 
> >
> > I would have expected that we only request preemption if we failed to 
> > schedule all tasks in the current group.  If I remember correctly, 
> > preemption slot search only happens on a per-group basis anyway.
> > 
> > You have probably thought about this, so I would like to understand 
> > your reasoning.
> 
> Maxim Khutornenko wrote:
> We still want to attempt a preemption if _some_ but not _all_ tasks are 
> scheduled within a given round, right? Otherwise, preemption becomes an 
> all-or-nothing feature and we have to wait for another scheduling cycle to 
> request a reservation.

Ok I see.


> On Sept. 16, 2016, 11:08 a.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java, line 174
> > 
> >
> > Shouldn't that only happen if `launchTask` has succeeded?
> 
> Maxim Khutornenko wrote:
> I've debated that as well and decided it's more logical to finish 
> accessing offer details before it's being launched with and removed from the 
> `OfferManager`.
> 
> BTW, I just realized I was missing a `break` statement to bail out from 
> the scheduling round if a `LaunchException` is thrown. While theoretically we 
> _could_ continue matching even after the `LaunchException`, it's hard to 
> reason about the state of the storage and the last assigned item and as such 
> it's safer to terminate than continue. Fixed and added a test case. This 
> should now be resolved. Thanks for asking :)

Thanks for the explanation. Makes sense to me.


- Stephan


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


On Sept. 16, 2016, 11:53 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51929/
> ---
> 
> (Updated Sept. 16, 2016, 11:53 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Stephan Erb, and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This is phase 2 of scheduling perf improvement effort started in 
> https://reviews.apache.org/r/51759/.
> 
> We can now take multiple (configurable) number of task IDs from a given 
> `TaskGroup` per scheduling. The idea is to go deeper through the offer queue 
> and assign more than one task if possible. This approach delivers 
> substantially better MTTA and still ensures fairness across multiple 
> `TaskGroups`. We have observed almost linear improvement in MTTA (4x+ with 5 
> tasks per round), which

Re: Review Request 52087: Fix host maintenance commands to properly initialize the api client.

2016-09-20 Thread Dmitriy Shirchenko 

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


Ship it!




Ship It!

- Dmitriy Shirchenko


On Sept. 20, 2016, 6:57 p.m., Joshua Cohen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52087/
> ---
> 
> (Updated Sept. 20, 2016, 6:57 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Zameer Manji.
> 
> 
> Bugs: AURORA-1777
> https://issues.apache.org/jira/browse/AURORA-1777
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Fix host maintenance commands to properly initialize the api client.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/admin/admin.py 
> 9fc89a2842d4651ac10aa82118531c450f051712 
>   src/main/python/apache/aurora/admin/admin_util.py 
> 394deb57af9ad8832a02ceab15f33b3c1e5c902b 
>   src/main/python/apache/aurora/admin/host_maintenance.py 
> 677f870c50d1e19d34de36a3c677bb1dcd255ba0 
>   src/main/python/apache/aurora/admin/maintenance.py 
> bf446515222a1246b7fe32acd67315f8145a1212 
>   src/test/python/apache/aurora/admin/test_admin.py 
> f720742c50f774b9f5a41ba57ef251d08d79e8d1 
>   src/test/python/apache/aurora/admin/test_admin_sla.py 
> 54b5a823903b82c2082353ade132eb7b63e518db 
>   src/test/python/apache/aurora/admin/test_admin_util.py 
> f5c8c69c1109d15ee3886fb863014c3285240db1 
>   src/test/python/apache/aurora/admin/util.py 
> d0a915cd8edee6b36d671b41f43a3af4fe751ae7 
>   src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 
> e36726e0c0154e51bba40704e13eebec5c0f0d65 
> 
> Diff: https://reviews.apache.org/r/52087/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> ran e2e tests.
> 
> 
> Thanks,
> 
> Joshua Cohen
> 
>

Re: Review Request 52087: Fix host maintenance commands to properly initialize the api client.

2016-09-20 Thread Joshua Cohen 

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

(Updated Sept. 20, 2016, 6:57 p.m.)


Review request for Aurora, Maxim Khutornenko and Zameer Manji.


Changes
---

Address review feedback.


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


Repository: aurora


Description
---

Fix host maintenance commands to properly initialize the api client.


Diffs (updated)
-

  src/main/python/apache/aurora/admin/admin.py 
9fc89a2842d4651ac10aa82118531c450f051712 
  src/main/python/apache/aurora/admin/admin_util.py 
394deb57af9ad8832a02ceab15f33b3c1e5c902b 
  src/main/python/apache/aurora/admin/host_maintenance.py 
677f870c50d1e19d34de36a3c677bb1dcd255ba0 
  src/main/python/apache/aurora/admin/maintenance.py 
bf446515222a1246b7fe32acd67315f8145a1212 
  src/test/python/apache/aurora/admin/test_admin.py 
f720742c50f774b9f5a41ba57ef251d08d79e8d1 
  src/test/python/apache/aurora/admin/test_admin_sla.py 
54b5a823903b82c2082353ade132eb7b63e518db 
  src/test/python/apache/aurora/admin/test_admin_util.py 
f5c8c69c1109d15ee3886fb863014c3285240db1 
  src/test/python/apache/aurora/admin/util.py 
d0a915cd8edee6b36d671b41f43a3af4fe751ae7 
  src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 
e36726e0c0154e51bba40704e13eebec5c0f0d65 

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


Testing
---

./build-support/jenkins/build.sh
ran e2e tests.


Thanks,

Joshua Cohen

Re: Review Request 51929: Scheduling multiple tasks per round.

2016-09-20 Thread Stephan Erb 

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




src/main/java/org/apache/aurora/scheduler/scheduling/SchedulingModule.java 
(lines 93 - 96)


Regarding your notes in the RB description: I don't see a problem if we set 
this to a slightly higher value such as `10` or `15`. 

It seems like we will maintain the basic taskgroup round robin scheduling 
fairness even with slightly larger batch sizes, so I am ok with bumping the 
value.



src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroups.java (lines 173 
- 174)


If I understand things correctly, I believe this line could have a 
performance bug in it:

`batchWorker.execute` acquires the global storage lock before calling the 
`taskScheduler`. For the latter, we use the following definition:

```
this.taskScheduler = (store, taskIds) -> {
  settings.rateLimiter.acquire();
  return taskScheduler.schedule(store, taskIds);
};
```

In combination, we will be throttled by the `rateLimiter` while holding the 
storage lock.

Instead, we should try to acquire the log within the `Runnable` in 
`startGroup` before calling `batchWorker.execute` so that the global lock is 
not hold longer than absolutely necessary.


- Stephan Erb


On Sept. 16, 2016, 11:53 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51929/
> ---
> 
> (Updated Sept. 16, 2016, 11:53 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Stephan Erb, and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This is phase 2 of scheduling perf improvement effort started in 
> https://reviews.apache.org/r/51759/.
> 
> We can now take multiple (configurable) number of task IDs from a given 
> `TaskGroup` per scheduling. The idea is to go deeper through the offer queue 
> and assign more than one task if possible. This approach delivers 
> substantially better MTTA and still ensures fairness across multiple 
> `TaskGroups`. We have observed almost linear improvement in MTTA (4x+ with 5 
> tasks per round), which suggest the `max_tasks_per_schedule_attempt` can be 
> set even higher if the majority of cluster jobs have large number of 
> instances and/or update batch sizes.
> 
> As far as a single round perf goes, we can consider the following 2 
> worst-case scenarios:
> - master: single task scheduling fails after trying all offers in the queue
> - this patch: N tasks launched with the very last N offers in the queue + `(N 
> x single_task_launch_latency)`
> 
> Assuming that matching N tasks against M offers takes exactly the same time 
> as 1 task against M offers (as they all share the same `TaskGroup`), the only 
> measurable difference comes from the additional `N x 
> single_task_launch_latency` overhead. Based on real cluster observations, the 
> `single_task_launch_latency` is less than 1% of a single task scheduling 
> attempt, which is << than the savings from avoided additional scheduling 
> rounds. 
> 
> As far as jmh results go, the new approach (batching + multiple tasks per 
> round) is only slightly more demanding (~8%). Both results though are MUCH 
> higher than the real cluster perf, which just confirms we are not bound by 
> CPU time here:
> 
> Master:
> ```
> Benchmark
> Mode  Cnt  Score Error  Units
> SchedulingBenchmarks.InsufficientResourcesSchedulingBenchmark.runBenchmark  
> thrpt   10  17126.183 ± 488.425  ops/s
> ```
> 
> This patch:
> ```
> Benchmark
> Mode  Cnt  Score Error  Units
> SchedulingBenchmarks.InsufficientResourcesSchedulingBenchmark.runBenchmark  
> thrpt   10  15838.051 ± 187.890  ops/s
> ```
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java 
> 6f1cbfbc4510a037cffc95fee54f62f463d2b534 
>   src/main/java/org/apache/aurora/scheduler/filter/AttributeAggregate.java 
> 87b9e1928ab2d44668df1123f32ffdc4197c0c70 
>   src/main/java/org/apache/aurora/scheduler/scheduling/SchedulingModule.java 
> 664bc6cf964ede2473a4463e58bcdbcb65bc7413 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java 
> 5d319557057e27fd5fc6d3e553e9ca9139399c50 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroups.java 
> d390c07522d22e43d79ce4370985f3643ef021ca 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskScheduler.java 
> 207d38d1ddfd373892602218a98c1daaf4a1325f 
>

Re: Review Request 52087: Fix host maintenance commands to properly initialize the api client.

2016-09-20 Thread Joshua Cohen 


> On Sept. 20, 2016, 5:53 p.m., Dmitriy Shirchenko wrote:
> > src/main/python/apache/aurora/admin/admin_util.py, line 272
> > 
> >
> > docstring, please!

Added.


> On Sept. 20, 2016, 5:53 p.m., Dmitriy Shirchenko wrote:
> > src/main/python/apache/aurora/admin/admin_util.py, line 273
> > 
> >
> > isinstance is preferred to type since isinstance keeps track of 
> > inheritance.

Good point, fixed.


> On Sept. 20, 2016, 5:53 p.m., Dmitriy Shirchenko wrote:
> > src/main/python/apache/aurora/admin/admin_util.py, line 276
> > 
> >
> > is there a test for this switch?

Thanks for the nudge, added a few test cases.


> On Sept. 20, 2016, 5:53 p.m., Dmitriy Shirchenko wrote:
> > src/test/python/apache/aurora/admin/test_admin.py, line 232
> > 
> >
> > is there a mock with bypass_leader_redirect = True?

I think there's sufficient coverage for this on the client directly. I don't 
think more coverage here buys us anything more than asserting we pass the value 
on from options which is of dubious value.


- Joshua


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


On Sept. 20, 2016, 6:41 p.m., Joshua Cohen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52087/
> ---
> 
> (Updated Sept. 20, 2016, 6:41 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Zameer Manji.
> 
> 
> Bugs: AURORA-1777
> https://issues.apache.org/jira/browse/AURORA-1777
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Fix host maintenance commands to properly initialize the api client.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/admin/admin.py 
> 9fc89a2842d4651ac10aa82118531c450f051712 
>   src/main/python/apache/aurora/admin/admin_util.py 
> 394deb57af9ad8832a02ceab15f33b3c1e5c902b 
>   src/main/python/apache/aurora/admin/host_maintenance.py 
> 677f870c50d1e19d34de36a3c677bb1dcd255ba0 
>   src/main/python/apache/aurora/admin/maintenance.py 
> bf446515222a1246b7fe32acd67315f8145a1212 
>   src/test/python/apache/aurora/admin/test_admin.py 
> f720742c50f774b9f5a41ba57ef251d08d79e8d1 
>   src/test/python/apache/aurora/admin/test_admin_sla.py 
> 54b5a823903b82c2082353ade132eb7b63e518db 
>   src/test/python/apache/aurora/admin/util.py 
> d0a915cd8edee6b36d671b41f43a3af4fe751ae7 
>   src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 
> e36726e0c0154e51bba40704e13eebec5c0f0d65 
> 
> Diff: https://reviews.apache.org/r/52087/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> ran e2e tests.
> 
> 
> Thanks,
> 
> Joshua Cohen
> 
>

Re: Review Request 52087: Fix host maintenance commands to properly initialize the api client.

2016-09-20 Thread Joshua Cohen 

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

(Updated Sept. 20, 2016, 6:41 p.m.)


Review request for Aurora, Maxim Khutornenko and Zameer Manji.


Changes
---

+bug


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


Repository: aurora


Description
---

Fix host maintenance commands to properly initialize the api client.


Diffs
-

  src/main/python/apache/aurora/admin/admin.py 
9fc89a2842d4651ac10aa82118531c450f051712 
  src/main/python/apache/aurora/admin/admin_util.py 
394deb57af9ad8832a02ceab15f33b3c1e5c902b 
  src/main/python/apache/aurora/admin/host_maintenance.py 
677f870c50d1e19d34de36a3c677bb1dcd255ba0 
  src/main/python/apache/aurora/admin/maintenance.py 
bf446515222a1246b7fe32acd67315f8145a1212 
  src/test/python/apache/aurora/admin/test_admin.py 
f720742c50f774b9f5a41ba57ef251d08d79e8d1 
  src/test/python/apache/aurora/admin/test_admin_sla.py 
54b5a823903b82c2082353ade132eb7b63e518db 
  src/test/python/apache/aurora/admin/util.py 
d0a915cd8edee6b36d671b41f43a3af4fe751ae7 
  src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 
e36726e0c0154e51bba40704e13eebec5c0f0d65 

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


Testing
---

./build-support/jenkins/build.sh
ran e2e tests.


Thanks,

Joshua Cohen

Re: Review Request 52074: switching from launchTask to acceptOffers

2016-09-20 Thread Aurora ReviewBot 

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


Ship it!




Master (4745c8c) 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 Sept. 20, 2016, 6:08 p.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52074/
> ---
> 
> (Updated Sept. 20, 2016, 6:08 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Mehrdad Nurolahzade, and Zameer 
> Manji.
> 
> 
> Bugs: AURORA-1776
> https://issues.apache.org/jira/browse/AURORA-1776
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Dynamic reservation requires us to use acceptOffers so this is in preparation 
> of that work. acceptOffers is the call to dynamically reserve resources.
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeDriver.java 
> 1bfd8dbf3aec314aaeb89b756ea3a2049df8777a 
>   src/main/java/org/apache/aurora/scheduler/mesos/Driver.java 
> 14481468926b82a9bad3ee78c85f176e8ba893e2 
>   src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverService.java 
> 41b9aab55aa439b13eba3545703ec97b44690444 
>   src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 
> 925c025d21bf1d44e0c1d319f6653ecfa8899481 
>   src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java 
> a739bce1226d9435fa7d0b18e411064a4e78e49e 
> 
> Diff: https://reviews.apache.org/r/52074/diff/
> 
> 
> Testing
> ---
> 
> src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 52087: Fix host maintenance commands to properly initialize the api client.

2016-09-20 Thread Zameer Manji 

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


Ship it!




Don't forget to fill out the `Bugs` portion of the review to link to the JIRA 
ticket.

I greatly appreciate the addition to the e2e test.

- Zameer Manji


On Sept. 20, 2016, 10:32 a.m., Joshua Cohen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52087/
> ---
> 
> (Updated Sept. 20, 2016, 10:32 a.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Fix host maintenance commands to properly initialize the api client.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/admin/admin.py 
> 9fc89a2842d4651ac10aa82118531c450f051712 
>   src/main/python/apache/aurora/admin/admin_util.py 
> 394deb57af9ad8832a02ceab15f33b3c1e5c902b 
>   src/main/python/apache/aurora/admin/host_maintenance.py 
> 677f870c50d1e19d34de36a3c677bb1dcd255ba0 
>   src/main/python/apache/aurora/admin/maintenance.py 
> bf446515222a1246b7fe32acd67315f8145a1212 
>   src/test/python/apache/aurora/admin/test_admin.py 
> f720742c50f774b9f5a41ba57ef251d08d79e8d1 
>   src/test/python/apache/aurora/admin/test_admin_sla.py 
> 54b5a823903b82c2082353ade132eb7b63e518db 
>   src/test/python/apache/aurora/admin/util.py 
> d0a915cd8edee6b36d671b41f43a3af4fe751ae7 
>   src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 
> e36726e0c0154e51bba40704e13eebec5c0f0d65 
> 
> Diff: https://reviews.apache.org/r/52087/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> ran e2e tests.
> 
> 
> Thanks,
> 
> Joshua Cohen
> 
>

Re: Review Request 52074: switching from launchTask to acceptOffers

2016-09-20 Thread Dmitriy Shirchenko 

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

(Updated Sept. 20, 2016, 6:08 p.m.)


Review request for Aurora, Maxim Khutornenko, Mehrdad Nurolahzade, and Zameer 
Manji.


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


Repository: aurora


Description
---

Dynamic reservation requires us to use acceptOffers so this is in preparation 
of that work. acceptOffers is the call to dynamically reserve resources.


Diffs (updated)
-

  src/jmh/java/org/apache/aurora/benchmark/fakes/FakeDriver.java 
1bfd8dbf3aec314aaeb89b756ea3a2049df8777a 
  src/main/java/org/apache/aurora/scheduler/mesos/Driver.java 
14481468926b82a9bad3ee78c85f176e8ba893e2 
  src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverService.java 
41b9aab55aa439b13eba3545703ec97b44690444 
  src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 
925c025d21bf1d44e0c1d319f6653ecfa8899481 
  src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java 
a739bce1226d9435fa7d0b18e411064a4e78e49e 

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


Testing
---

src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh


Thanks,

Dmitriy Shirchenko

Re: Review Request 52087: Fix host maintenance commands to properly initialize the api client.

2016-09-20 Thread Aurora ReviewBot 

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



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

 # Create file stdout for capturing output. We 
can't use StringIO mock
 # because TestProcess is running fork.
 with open(os.path.join(td, 'sys_stdout'), 
'w+') as stdout:
   with open(os.path.join(td, 'sys_stderr'), 
'w+') as stderr:
 with mutable_sys():
   sys.stdout, sys.stderr = stdout, 
stderr
 
   p = TestProcess('process', 'echo hello 
world; echo >&2 hello stderr', 0,
   taskpath, sandbox, 
logger_destination=LoggerDestination.BOTH)
   p.start()
   rc = 
wait_for_rc(taskpath.getpath('process_checkpoint'))
 
   assert rc == 0
   # Check log files were created in std 
path with correct content
 > assert_log_content(taskpath, 'stdout', 
'hello world\n')
 
 src/test/python/apache/thermos/core/test_process.py:487: 
 _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
 
 taskpath = 
 log_name = 'stdout'
 expected_content = 'hello world\n'
 
 def assert_log_content(taskpath, log_name, 
expected_content):
   log = 
taskpath.with_filename(log_name).getpath('process_logdir')
   assert os.path.exists(log)
   with open(log, 'r') as fp:
 >   assert fp.read() == expected_content
 E   assert '' == 'hello world\n'
 E + hello world
 
 src/test/python/apache/thermos/core/test_process.py:313: 
AssertionError
  generated xml file: 
/home/jenkins/jenkins-slave/workspace/AuroraBot/dist/test-results/415337499eb72578eab327a6487c1f5c9452b3d6.xml
 
  1 failed, 710 passed, 6 skipped, 1 warnings in 
402.35 seconds 
 
FAILURE


17:53:10 07:30   [complete]
   FAILURE


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

- Aurora ReviewBot


On Sept. 20, 2016, 5:32 p.m., Joshua Cohen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52087/
> ---
> 
> (Updated Sept. 20, 2016, 5:32 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Fix host maintenance commands to properly initialize the api client.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/admin/admin.py 
> 9fc89a2842d4651ac10aa82118531c450f051712 
>   src/main/python/apache/aurora/admin/admin_util.py 
> 394deb57af9ad8832a02ceab15f33b3c1e5c902b 
>   src/main/python/apache/aurora/admin/host_maintenance.py 
> 677f870c50d1e19d34de36a3c677bb1dcd255ba0 
>   src/main/python/apache/aurora/admin/maintenance.py 
> bf446515222a1246b7fe32acd67315f8145a1212 
>   src/test/python/apache/aurora/admin/test_admin.py 
> f720742c50f774b9f5a41ba57ef251d08d79e8d1 
>   src/test/python/apache/aurora/admin/test_admin_sla.py 
> 54b5a823903b82c2082353ade132eb7b63e518db 
>   src/test/python/apache/aurora/admin/util.py 
> d0a915cd8edee6b36d671b41f43a3af4fe751ae7 
>   src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 
> e36726e0c0154e51bba40704e13eebec5c0f0d65 
> 
> Diff: https://reviews.apache.org/r/52087/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> ran e2e tests.
> 
> 
> Thanks,
> 
> Joshua Cohen
> 
>

Re: Review Request 52087: Fix host maintenance commands to properly initialize the api client.

2016-09-20 Thread Dmitriy Shirchenko 

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




src/main/python/apache/aurora/admin/admin_util.py (line 272)


docstring, please!



src/main/python/apache/aurora/admin/admin_util.py (line 273)


isinstance is preferred to type since isinstance keeps track of inheritance.



src/main/python/apache/aurora/admin/admin_util.py (line 276)


is there a test for this switch?



src/test/python/apache/aurora/admin/test_admin.py (line 232)


is there a mock with bypass_leader_redirect = True?


- Dmitriy Shirchenko


On Sept. 20, 2016, 5:32 p.m., Joshua Cohen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52087/
> ---
> 
> (Updated Sept. 20, 2016, 5:32 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Fix host maintenance commands to properly initialize the api client.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/admin/admin.py 
> 9fc89a2842d4651ac10aa82118531c450f051712 
>   src/main/python/apache/aurora/admin/admin_util.py 
> 394deb57af9ad8832a02ceab15f33b3c1e5c902b 
>   src/main/python/apache/aurora/admin/host_maintenance.py 
> 677f870c50d1e19d34de36a3c677bb1dcd255ba0 
>   src/main/python/apache/aurora/admin/maintenance.py 
> bf446515222a1246b7fe32acd67315f8145a1212 
>   src/test/python/apache/aurora/admin/test_admin.py 
> f720742c50f774b9f5a41ba57ef251d08d79e8d1 
>   src/test/python/apache/aurora/admin/test_admin_sla.py 
> 54b5a823903b82c2082353ade132eb7b63e518db 
>   src/test/python/apache/aurora/admin/util.py 
> d0a915cd8edee6b36d671b41f43a3af4fe751ae7 
>   src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 
> e36726e0c0154e51bba40704e13eebec5c0f0d65 
> 
> Diff: https://reviews.apache.org/r/52087/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> ran e2e tests.
> 
> 
> Thanks,
> 
> Joshua Cohen
> 
>

Re: Review Request 52087: Fix host maintenance commands to properly initialize the api client.

2016-09-20 Thread Maxim Khutornenko 

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


Ship it!




Ship It!

- Maxim Khutornenko


On Sept. 20, 2016, 5:32 p.m., Joshua Cohen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52087/
> ---
> 
> (Updated Sept. 20, 2016, 5:32 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Fix host maintenance commands to properly initialize the api client.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/admin/admin.py 
> 9fc89a2842d4651ac10aa82118531c450f051712 
>   src/main/python/apache/aurora/admin/admin_util.py 
> 394deb57af9ad8832a02ceab15f33b3c1e5c902b 
>   src/main/python/apache/aurora/admin/host_maintenance.py 
> 677f870c50d1e19d34de36a3c677bb1dcd255ba0 
>   src/main/python/apache/aurora/admin/maintenance.py 
> bf446515222a1246b7fe32acd67315f8145a1212 
>   src/test/python/apache/aurora/admin/test_admin.py 
> f720742c50f774b9f5a41ba57ef251d08d79e8d1 
>   src/test/python/apache/aurora/admin/test_admin_sla.py 
> 54b5a823903b82c2082353ade132eb7b63e518db 
>   src/test/python/apache/aurora/admin/util.py 
> d0a915cd8edee6b36d671b41f43a3af4fe751ae7 
>   src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 
> e36726e0c0154e51bba40704e13eebec5c0f0d65 
> 
> Diff: https://reviews.apache.org/r/52087/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> ran e2e tests.
> 
> 
> Thanks,
> 
> Joshua Cohen
> 
>

Review Request 52087: Fix host maintenance commands to properly initialize the api client.

2016-09-20 Thread Joshua Cohen 

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

Review request for Aurora, Maxim Khutornenko and Zameer Manji.


Repository: aurora


Description
---

Fix host maintenance commands to properly initialize the api client.


Diffs
-

  src/main/python/apache/aurora/admin/admin.py 
9fc89a2842d4651ac10aa82118531c450f051712 
  src/main/python/apache/aurora/admin/admin_util.py 
394deb57af9ad8832a02ceab15f33b3c1e5c902b 
  src/main/python/apache/aurora/admin/host_maintenance.py 
677f870c50d1e19d34de36a3c677bb1dcd255ba0 
  src/main/python/apache/aurora/admin/maintenance.py 
bf446515222a1246b7fe32acd67315f8145a1212 
  src/test/python/apache/aurora/admin/test_admin.py 
f720742c50f774b9f5a41ba57ef251d08d79e8d1 
  src/test/python/apache/aurora/admin/test_admin_sla.py 
54b5a823903b82c2082353ade132eb7b63e518db 
  src/test/python/apache/aurora/admin/util.py 
d0a915cd8edee6b36d671b41f43a3af4fe751ae7 
  src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 
e36726e0c0154e51bba40704e13eebec5c0f0d65 

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


Testing
---

./build-support/jenkins/build.sh
ran e2e tests.


Thanks,

Joshua Cohen

Re: Review Request 51980: Refactor of Webhook and no longer posting entire task state via webhook on scheduler restart

2016-09-20 Thread Stephan Erb 

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


Ship it!




Ship It!

- Stephan Erb


On Sept. 19, 2016, 8:28 p.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51980/
> ---
> 
> (Updated Sept. 19, 2016, 8:28 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Stephan Erb, and Zameer Manji.
> 
> 
> Bugs: AURORA-1772
> https://issues.apache.org/jira/browse/AURORA-1772
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This is a refactor with addition of HttpClient injected into Webhook class as 
> opposed to previous usage of lower level HtttpURLConnection objects. 
> Additionally due to peformance issues, it is unncessary to POST entire task 
> state to webhook endpoint on every scheduler restart so that is removed in 
> this patch.
> 
> 
> Diffs
> -
> 
>   build.gradle d5a3a7a3bdb8349de6fc01d4a6271b32d942e531 
>   src/main/java/org/apache/aurora/scheduler/events/Webhook.java 
> e54aa19d67278fcb5586f07c399f09062f845a18 
>   src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java 
> e4193f7518e66d210a6d5aae22a9f04e2d4984b3 
>   src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java 
> eaa70533a64740c74cebf15739b7142e2d3a4d11 
>   src/main/resources/org/apache/aurora/scheduler/webhook.json 
> 00787985510d7d415b8074bef06d28b635c78b09 
>   src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java 
> 488eefd14c3e67a41a75c809397c8d19f83cc08a 
> 
> Diff: https://reviews.apache.org/r/51980/diff/
> 
> 
> Testing
> ---
> 
> Part of reason for refactor is to allow easier testing so cleaner unit tests 
> were added with more code coverage.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 51980: Refactor of Webhook and no longer posting entire task state via webhook on scheduler restart

2016-09-20 Thread Maxim Khutornenko 

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


Ship it!




Ship It!

- Maxim Khutornenko


On Sept. 19, 2016, 6:28 p.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51980/
> ---
> 
> (Updated Sept. 19, 2016, 6:28 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Stephan Erb, and Zameer Manji.
> 
> 
> Bugs: AURORA-1772
> https://issues.apache.org/jira/browse/AURORA-1772
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This is a refactor with addition of HttpClient injected into Webhook class as 
> opposed to previous usage of lower level HtttpURLConnection objects. 
> Additionally due to peformance issues, it is unncessary to POST entire task 
> state to webhook endpoint on every scheduler restart so that is removed in 
> this patch.
> 
> 
> Diffs
> -
> 
>   build.gradle d5a3a7a3bdb8349de6fc01d4a6271b32d942e531 
>   src/main/java/org/apache/aurora/scheduler/events/Webhook.java 
> e54aa19d67278fcb5586f07c399f09062f845a18 
>   src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java 
> e4193f7518e66d210a6d5aae22a9f04e2d4984b3 
>   src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java 
> eaa70533a64740c74cebf15739b7142e2d3a4d11 
>   src/main/resources/org/apache/aurora/scheduler/webhook.json 
> 00787985510d7d415b8074bef06d28b635c78b09 
>   src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java 
> 488eefd14c3e67a41a75c809397c8d19f83cc08a 
> 
> Diff: https://reviews.apache.org/r/51980/diff/
> 
> 
> Testing
> ---
> 
> Part of reason for refactor is to allow easier testing so cleaner unit tests 
> were added with more code coverage.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>