Re: Review Request 33608: Added a status update throughput benchmark.

2015-05-05 Thread Aurora ReviewBot 

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


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

However, it appears that it might lack test coverage.

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

- Aurora ReviewBot


On May 5, 2015, 10:24 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33608/
> ---
> 
> (Updated May 5, 2015, 10:24 p.m.)
> 
> 
> Review request for Aurora and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1283
> https://issues.apache.org/jira/browse/AURORA-1283
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> In order to justify doing asynchronous batch acknowledgements and to better 
> understand status update throughput, this introduces a benchmark.
> 
> Note that this assumes that status update processing is not synchronous, so 
> that the benchmark doesn't need to be updated for AURORA-1228.
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeOfferManager.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 
> c54619f7cd617b48069363173dcf63b6254b4095 
>   src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 
> d7d659bb13f085ff06291ef0033572f8bdf29874 
> 
> Diff: https://reviews.apache.org/r/33608/diff/
> 
> 
> Testing
> ---
> 
> Ran the benchmarks against the existing code and some pending code I have for 
> AURORA-1228 to demonstrate the improvement.
> 
> The end to end tests are broken, appears to be unrelated to my change from 
> what I can tell.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 33608: Added a status update throughput benchmark.

2015-05-05 Thread Ben Mahler 

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

(Updated May 5, 2015, 10:24 p.m.)


Review request for Aurora and Maxim Khutornenko.


Changes
---

Added a maybeSleep() per Bill's request.


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


Repository: aurora


Description
---

In order to justify doing asynchronous batch acknowledgements and to better 
understand status update throughput, this introduces a benchmark.

Note that this assumes that status update processing is not synchronous, so 
that the benchmark doesn't need to be updated for AURORA-1228.


Diffs (updated)
-

  src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java 
PRE-CREATION 
  src/jmh/java/org/apache/aurora/benchmark/fakes/FakeOfferManager.java 
PRE-CREATION 
  src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 
c54619f7cd617b48069363173dcf63b6254b4095 
  src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 
d7d659bb13f085ff06291ef0033572f8bdf29874 

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


Testing
---

Ran the benchmarks against the existing code and some pending code I have for 
AURORA-1228 to demonstrate the improvement.

The end to end tests are broken, appears to be unrelated to my change from what 
I can tell.


Thanks,

Ben Mahler

Re: Review Request 33608: Added a status update throughput benchmark.

2015-05-05 Thread Bill Farner 

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

Ship it!



src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java


DRY - extract a method:

```
private void maybeSleep() {
  if (latency.isPresent()) {
...
  }
}
```


- Bill Farner


On April 30, 2015, 12:35 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33608/
> ---
> 
> (Updated April 30, 2015, 12:35 a.m.)
> 
> 
> Review request for Aurora and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1283
> https://issues.apache.org/jira/browse/AURORA-1283
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> In order to justify doing asynchronous batch acknowledgements and to better 
> understand status update throughput, this introduces a benchmark.
> 
> Note that this assumes that status update processing is not synchronous, so 
> that the benchmark doesn't need to be updated for AURORA-1228.
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeOfferManager.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 
> c54619f7cd617b48069363173dcf63b6254b4095 
>   src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 
> d7d659bb13f085ff06291ef0033572f8bdf29874 
> 
> Diff: https://reviews.apache.org/r/33608/diff/
> 
> 
> Testing
> ---
> 
> Ran the benchmarks against the existing code and some pending code I have for 
> AURORA-1228 to demonstrate the improvement.
> 
> The end to end tests are broken, appears to be unrelated to my change from 
> what I can tell.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 33608: Added a status update throughput benchmark.

2015-05-01 Thread Ben Mahler 


> On April 30, 2015, 5:29 p.m., Bill Farner wrote:
> > src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java, line 
> > 103
> > 
> >
> > I should have asked the first time around - what's the thought process 
> > behind including this?  Given that this is a benchmark, it seems only to 
> > place a ceiling on throughput.

My goal here was not to micro-benchmark the status update processing path, but 
to show how to achieve high throughput against an expensive resource. So, 
although it's obviously the case today that the latency bounds throughput, this 
benchmark serves to demonstrate that _even in the face of degraded storage 
latency_ we can have high throughput. With my upcoming changes, that is. :)


- Ben


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


On April 30, 2015, 12:35 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33608/
> ---
> 
> (Updated April 30, 2015, 12:35 a.m.)
> 
> 
> Review request for Aurora and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1283
> https://issues.apache.org/jira/browse/AURORA-1283
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> In order to justify doing asynchronous batch acknowledgements and to better 
> understand status update throughput, this introduces a benchmark.
> 
> Note that this assumes that status update processing is not synchronous, so 
> that the benchmark doesn't need to be updated for AURORA-1228.
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeOfferManager.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 
> c54619f7cd617b48069363173dcf63b6254b4095 
>   src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 
> d7d659bb13f085ff06291ef0033572f8bdf29874 
> 
> Diff: https://reviews.apache.org/r/33608/diff/
> 
> 
> Testing
> ---
> 
> Ran the benchmarks against the existing code and some pending code I have for 
> AURORA-1228 to demonstrate the improvement.
> 
> The end to end tests are broken, appears to be unrelated to my change from 
> what I can tell.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 33608: Added a status update throughput benchmark.

2015-04-30 Thread Bill Farner 

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



src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java


I should have asked the first time around - what's the thought process 
behind including this?  Given that this is a benchmark, it seems only to place 
a ceiling on throughput.


- Bill Farner


On April 30, 2015, 12:35 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33608/
> ---
> 
> (Updated April 30, 2015, 12:35 a.m.)
> 
> 
> Review request for Aurora and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1283
> https://issues.apache.org/jira/browse/AURORA-1283
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> In order to justify doing asynchronous batch acknowledgements and to better 
> understand status update throughput, this introduces a benchmark.
> 
> Note that this assumes that status update processing is not synchronous, so 
> that the benchmark doesn't need to be updated for AURORA-1228.
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeOfferManager.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 
> c54619f7cd617b48069363173dcf63b6254b4095 
>   src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 
> d7d659bb13f085ff06291ef0033572f8bdf29874 
> 
> Diff: https://reviews.apache.org/r/33608/diff/
> 
> 
> Testing
> ---
> 
> Ran the benchmarks against the existing code and some pending code I have for 
> AURORA-1228 to demonstrate the improvement.
> 
> The end to end tests are broken, appears to be unrelated to my change from 
> what I can tell.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 33608: Added a status update throughput benchmark.

2015-04-29 Thread Aurora ReviewBot 

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


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

However, it appears that it might lack test coverage.

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

- Aurora ReviewBot


On April 30, 2015, 12:35 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33608/
> ---
> 
> (Updated April 30, 2015, 12:35 a.m.)
> 
> 
> Review request for Aurora and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1283
> https://issues.apache.org/jira/browse/AURORA-1283
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> In order to justify doing asynchronous batch acknowledgements and to better 
> understand status update throughput, this introduces a benchmark.
> 
> Note that this assumes that status update processing is not synchronous, so 
> that the benchmark doesn't need to be updated for AURORA-1228.
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeOfferManager.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 
> c54619f7cd617b48069363173dcf63b6254b4095 
>   src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 
> d7d659bb13f085ff06291ef0033572f8bdf29874 
> 
> Diff: https://reviews.apache.org/r/33608/diff/
> 
> 
> Testing
> ---
> 
> Ran the benchmarks against the existing code and some pending code I have for 
> AURORA-1228 to demonstrate the improvement.
> 
> The end to end tests are broken, appears to be unrelated to my change from 
> what I can tell.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 33608: Added a status update throughput benchmark.

2015-04-29 Thread Ben Mahler 


> On April 29, 2015, 10:38 p.m., Bill Farner wrote:
> > src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java, line 
> > 179
> > 
> >
> > This requires libmesos.so to be available, which i don't think we 
> > should do.  Can you get away without a real `MesosSchedulerDriver`?

Ah, removing it complains about having no DriverFactory, so I've bound an 
implementation that just returns a fake scheduler driver. Let me know how that 
looks to you.


- Ben


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


On April 29, 2015, 6:12 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33608/
> ---
> 
> (Updated April 29, 2015, 6:12 p.m.)
> 
> 
> Review request for Aurora and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1283
> https://issues.apache.org/jira/browse/AURORA-1283
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> In order to justify doing asynchronous batch acknowledgements and to better 
> understand status update throughput, this introduces a benchmark.
> 
> Note that this assumes that status update processing is not synchronous, so 
> that the benchmark doesn't need to be updated for AURORA-1228.
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeOfferManager.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 
> c54619f7cd617b48069363173dcf63b6254b4095 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java 
> 9b8ab7c1027731f9d3f6cae77b85272ea63354d4 
>   src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 
> d7d659bb13f085ff06291ef0033572f8bdf29874 
> 
> Diff: https://reviews.apache.org/r/33608/diff/
> 
> 
> Testing
> ---
> 
> Ran the benchmarks against the existing code and some pending code I have for 
> AURORA-1228 to demonstrate the improvement.
> 
> The end to end tests are broken, appears to be unrelated to my change from 
> what I can tell.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 33608: Added a status update throughput benchmark.

2015-04-29 Thread Ben Mahler 

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

(Updated April 30, 2015, 12:35 a.m.)


Review request for Aurora and Maxim Khutornenko.


Changes
---

Bill's feedback.


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


Repository: aurora


Description
---

In order to justify doing asynchronous batch acknowledgements and to better 
understand status update throughput, this introduces a benchmark.

Note that this assumes that status update processing is not synchronous, so 
that the benchmark doesn't need to be updated for AURORA-1228.


Diffs (updated)
-

  src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java 
PRE-CREATION 
  src/jmh/java/org/apache/aurora/benchmark/fakes/FakeOfferManager.java 
PRE-CREATION 
  src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 
c54619f7cd617b48069363173dcf63b6254b4095 
  src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 
d7d659bb13f085ff06291ef0033572f8bdf29874 

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


Testing
---

Ran the benchmarks against the existing code and some pending code I have for 
AURORA-1228 to demonstrate the improvement.

The end to end tests are broken, appears to be unrelated to my change from what 
I can tell.


Thanks,

Ben Mahler

Re: Review Request 33608: Added a status update throughput benchmark.

2015-04-29 Thread Ben Mahler 


> On April 29, 2015, 10:31 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java, 
> > line 45
> > 
> >
> > If anything, you should expose Scheduler.class to hide the 
> > implementation.  This should allow you to revert changes in 
> > MesosSchedulerImpl.java.

Done.


- Ben


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


On April 29, 2015, 6:12 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33608/
> ---
> 
> (Updated April 29, 2015, 6:12 p.m.)
> 
> 
> Review request for Aurora and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1283
> https://issues.apache.org/jira/browse/AURORA-1283
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> In order to justify doing asynchronous batch acknowledgements and to better 
> understand status update throughput, this introduces a benchmark.
> 
> Note that this assumes that status update processing is not synchronous, so 
> that the benchmark doesn't need to be updated for AURORA-1228.
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeOfferManager.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 
> c54619f7cd617b48069363173dcf63b6254b4095 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java 
> 9b8ab7c1027731f9d3f6cae77b85272ea63354d4 
>   src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 
> d7d659bb13f085ff06291ef0033572f8bdf29874 
> 
> Diff: https://reviews.apache.org/r/33608/diff/
> 
> 
> Testing
> ---
> 
> Ran the benchmarks against the existing code and some pending code I have for 
> AURORA-1228 to demonstrate the improvement.
> 
> The end to end tests are broken, appears to be unrelated to my change from 
> what I can tell.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 33608: Added a status update throughput benchmark.

2015-04-29 Thread Bill Farner 

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


Sorry, one last drive-by that is probably a ship-blocker, albeit minor.


src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java


This requires libmesos.so to be available, which i don't think we should 
do.  Can you get away without a real `MesosSchedulerDriver`?


- Bill Farner


On April 29, 2015, 6:12 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33608/
> ---
> 
> (Updated April 29, 2015, 6:12 p.m.)
> 
> 
> Review request for Aurora and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1283
> https://issues.apache.org/jira/browse/AURORA-1283
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> In order to justify doing asynchronous batch acknowledgements and to better 
> understand status update throughput, this introduces a benchmark.
> 
> Note that this assumes that status update processing is not synchronous, so 
> that the benchmark doesn't need to be updated for AURORA-1228.
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeOfferManager.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 
> c54619f7cd617b48069363173dcf63b6254b4095 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java 
> 9b8ab7c1027731f9d3f6cae77b85272ea63354d4 
>   src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 
> d7d659bb13f085ff06291ef0033572f8bdf29874 
> 
> Diff: https://reviews.apache.org/r/33608/diff/
> 
> 
> Testing
> ---
> 
> Ran the benchmarks against the existing code and some pending code I have for 
> AURORA-1228 to demonstrate the improvement.
> 
> The end to end tests are broken, appears to be unrelated to my change from 
> what I can tell.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 33608: Added a status update throughput benchmark.

2015-04-29 Thread Bill Farner 

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



src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java


If anything, you should expose Scheduler.class to hide the implementation.  
This should allow you to revert changes in MesosSchedulerImpl.java.


- Bill Farner


On April 29, 2015, 6:12 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33608/
> ---
> 
> (Updated April 29, 2015, 6:12 p.m.)
> 
> 
> Review request for Aurora and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1283
> https://issues.apache.org/jira/browse/AURORA-1283
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> In order to justify doing asynchronous batch acknowledgements and to better 
> understand status update throughput, this introduces a benchmark.
> 
> Note that this assumes that status update processing is not synchronous, so 
> that the benchmark doesn't need to be updated for AURORA-1228.
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeOfferManager.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 
> c54619f7cd617b48069363173dcf63b6254b4095 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java 
> 9b8ab7c1027731f9d3f6cae77b85272ea63354d4 
>   src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 
> d7d659bb13f085ff06291ef0033572f8bdf29874 
> 
> Diff: https://reviews.apache.org/r/33608/diff/
> 
> 
> Testing
> ---
> 
> Ran the benchmarks against the existing code and some pending code I have for 
> AURORA-1228 to demonstrate the improvement.
> 
> The end to end tests are broken, appears to be unrelated to my change from 
> what I can tell.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 33608: Added a status update throughput benchmark.

2015-04-29 Thread Aurora ReviewBot 

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


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

However, it appears that it might lack test coverage.

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

- Aurora ReviewBot


On April 29, 2015, 6:12 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33608/
> ---
> 
> (Updated April 29, 2015, 6:12 p.m.)
> 
> 
> Review request for Aurora and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1283
> https://issues.apache.org/jira/browse/AURORA-1283
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> In order to justify doing asynchronous batch acknowledgements and to better 
> understand status update throughput, this introduces a benchmark.
> 
> Note that this assumes that status update processing is not synchronous, so 
> that the benchmark doesn't need to be updated for AURORA-1228.
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeOfferManager.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 
> c54619f7cd617b48069363173dcf63b6254b4095 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java 
> 9b8ab7c1027731f9d3f6cae77b85272ea63354d4 
>   src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 
> d7d659bb13f085ff06291ef0033572f8bdf29874 
> 
> Diff: https://reviews.apache.org/r/33608/diff/
> 
> 
> Testing
> ---
> 
> Ran the benchmarks against the existing code and some pending code I have for 
> AURORA-1228 to demonstrate the improvement.
> 
> The end to end tests are broken, appears to be unrelated to my change from 
> what I can tell.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 33608: Added a status update throughput benchmark.

2015-04-29 Thread Maxim Khutornenko 

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

Ship it!


Ship It!

- Maxim Khutornenko


On April 29, 2015, 6:12 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33608/
> ---
> 
> (Updated April 29, 2015, 6:12 p.m.)
> 
> 
> Review request for Aurora and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1283
> https://issues.apache.org/jira/browse/AURORA-1283
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> In order to justify doing asynchronous batch acknowledgements and to better 
> understand status update throughput, this introduces a benchmark.
> 
> Note that this assumes that status update processing is not synchronous, so 
> that the benchmark doesn't need to be updated for AURORA-1228.
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeOfferManager.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 
> c54619f7cd617b48069363173dcf63b6254b4095 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java 
> 9b8ab7c1027731f9d3f6cae77b85272ea63354d4 
>   src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 
> d7d659bb13f085ff06291ef0033572f8bdf29874 
> 
> Diff: https://reviews.apache.org/r/33608/diff/
> 
> 
> Testing
> ---
> 
> Ran the benchmarks against the existing code and some pending code I have for 
> AURORA-1228 to demonstrate the improvement.
> 
> The end to end tests are broken, appears to be unrelated to my change from 
> what I can tell.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 33608: Added a status update throughput benchmark.

2015-04-29 Thread Ben Mahler 

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

(Updated April 29, 2015, 6:12 p.m.)


Review request for Aurora and Maxim Khutornenko.


Changes
---

Maxim's review.


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


Repository: aurora


Description
---

In order to justify doing asynchronous batch acknowledgements and to better 
understand status update throughput, this introduces a benchmark.

Note that this assumes that status update processing is not synchronous, so 
that the benchmark doesn't need to be updated for AURORA-1228.


Diffs (updated)
-

  src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java 
PRE-CREATION 
  src/jmh/java/org/apache/aurora/benchmark/fakes/FakeOfferManager.java 
PRE-CREATION 
  src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 
c54619f7cd617b48069363173dcf63b6254b4095 
  src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java 
9b8ab7c1027731f9d3f6cae77b85272ea63354d4 
  src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 
d7d659bb13f085ff06291ef0033572f8bdf29874 

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


Testing
---

Ran the benchmarks against the existing code and some pending code I have for 
AURORA-1228 to demonstrate the improvement.

The end to end tests are broken, appears to be unrelated to my change from what 
I can tell.


Thanks,

Ben Mahler

Re: Review Request 33608: Added a status update throughput benchmark.

2015-04-29 Thread Ben Mahler 


> On April 29, 2015, 1:42 a.m., Maxim Khutornenko wrote:
> > src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java, line 
> > 106
> > 
> >
> > s/public/private

Done.


> On April 29, 2015, 1:42 a.m., Maxim Khutornenko wrote:
> > src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java, line 
> > 107
> > 
> >
> > requireNonNull() for both

Done.


> On April 29, 2015, 1:42 a.m., Maxim Khutornenko wrote:
> > src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java, line 
> > 132
> > 
> >
> > add newline before

Done. This is I think where braces on the next line help, but that ship has 
sailed :)

https://twitter.com/blundell_apps/status/593390700332453888


> On April 29, 2015, 1:42 a.m., Maxim Khutornenko wrote:
> > src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java, line 
> > 149
> > 
> >
> > drop

Done.


> On April 29, 2015, 1:42 a.m., Maxim Khutornenko wrote:
> > src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java, line 
> > 155
> > 
> >
> > s/protected/private/g

Done.


> On April 29, 2015, 1:42 a.m., Maxim Khutornenko wrote:
> > src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java, line 
> > 183
> > 
> >
> > Suggest inlining the anonymous Command instead.

Done.


> On April 29, 2015, 1:42 a.m., Maxim Khutornenko wrote:
> > src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java, line 
> > 169
> > 
> >
> > This is no longer going to work the way you expect given that the setup 
> > level is Trial now. You'd need to have a setter in your storage wrapper 
> > that will be called from a new handler with `Level.Iteration`.

Ah, I see that now. Ran it to make sure this was behaving as expected.


- Ben


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


On April 29, 2015, 12:48 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33608/
> ---
> 
> (Updated April 29, 2015, 12:48 a.m.)
> 
> 
> Review request for Aurora and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1283
> https://issues.apache.org/jira/browse/AURORA-1283
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> In order to justify doing asynchronous batch acknowledgements and to better 
> understand status update throughput, this introduces a benchmark.
> 
> Note that this assumes that status update processing is not synchronous, so 
> that the benchmark doesn't need to be updated for AURORA-1228.
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeCommand.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeOfferManager.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 
> c54619f7cd617b48069363173dcf63b6254b4095 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java 
> 9b8ab7c1027731f9d3f6cae77b85272ea63354d4 
>   src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 
> d7d659bb13f085ff06291ef0033572f8bdf29874 
> 
> Diff: https://reviews.apache.org/r/33608/diff/
> 
> 
> Testing
> ---
> 
> Ran the benchmarks against the existing code and some pending code I have for 
> AURORA-1228 to demonstrate the improvement.
> 
> The end to end tests are broken, appears to be unrelated to my change from 
> what I can tell.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 33608: Added a status update throughput benchmark.

2015-04-28 Thread Maxim Khutornenko 

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


Looks good overall. A few comments left.


src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java


s/public/private



src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java


requireNonNull() for both



src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java


add newline before



src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java


drop



src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java


s/protected/private/g



src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java


This is no longer going to work the way you expect given that the setup 
level is Trial now. You'd need to have a setter in your storage wrapper that 
will be called from a new handler with `Level.Iteration`.



src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java


Suggest inlining the anonymous Command instead.


- Maxim Khutornenko


On April 29, 2015, 12:48 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33608/
> ---
> 
> (Updated April 29, 2015, 12:48 a.m.)
> 
> 
> Review request for Aurora and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1283
> https://issues.apache.org/jira/browse/AURORA-1283
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> In order to justify doing asynchronous batch acknowledgements and to better 
> understand status update throughput, this introduces a benchmark.
> 
> Note that this assumes that status update processing is not synchronous, so 
> that the benchmark doesn't need to be updated for AURORA-1228.
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeCommand.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeOfferManager.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 
> c54619f7cd617b48069363173dcf63b6254b4095 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java 
> 9b8ab7c1027731f9d3f6cae77b85272ea63354d4 
>   src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 
> d7d659bb13f085ff06291ef0033572f8bdf29874 
> 
> Diff: https://reviews.apache.org/r/33608/diff/
> 
> 
> Testing
> ---
> 
> Ran the benchmarks against the existing code and some pending code I have for 
> AURORA-1228 to demonstrate the improvement.
> 
> The end to end tests are broken, appears to be unrelated to my change from 
> what I can tell.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 33608: Added a status update throughput benchmark.

2015-04-28 Thread Aurora ReviewBot 

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


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

However, it appears that it might lack test coverage.

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

- Aurora ReviewBot


On April 29, 2015, 12:48 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33608/
> ---
> 
> (Updated April 29, 2015, 12:48 a.m.)
> 
> 
> Review request for Aurora and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1283
> https://issues.apache.org/jira/browse/AURORA-1283
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> In order to justify doing asynchronous batch acknowledgements and to better 
> understand status update throughput, this introduces a benchmark.
> 
> Note that this assumes that status update processing is not synchronous, so 
> that the benchmark doesn't need to be updated for AURORA-1228.
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeCommand.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeOfferManager.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 
> c54619f7cd617b48069363173dcf63b6254b4095 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java 
> 9b8ab7c1027731f9d3f6cae77b85272ea63354d4 
>   src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 
> d7d659bb13f085ff06291ef0033572f8bdf29874 
> 
> Diff: https://reviews.apache.org/r/33608/diff/
> 
> 
> Testing
> ---
> 
> Ran the benchmarks against the existing code and some pending code I have for 
> AURORA-1228 to demonstrate the improvement.
> 
> The end to end tests are broken, appears to be unrelated to my change from 
> what I can tell.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 33608: Added a status update throughput benchmark.

2015-04-28 Thread Ben Mahler 


> On April 28, 2015, 8:35 p.m., Maxim Khutornenko wrote:
> >

Thanks for the thorough review!


> On April 28, 2015, 8:35 p.m., Maxim Khutornenko wrote:
> > src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java, 
> > line 45
> > 
> >
> > Please, run ./gradlew -Pq build to pick up style warnings and run 
> > static analysis tools.

Thanks!


> On April 28, 2015, 8:35 p.m., Maxim Khutornenko wrote:
> > src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java, line 1
> > 
> >
> > Please, add Apache headers to all new files.

Done.


> On April 28, 2015, 8:35 p.m., Maxim Khutornenko wrote:
> > src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java, line 94
> > 
> >
> > s/public/private or move it into its own file.

Moved to private.


> On April 28, 2015, 8:35 p.m., Maxim Khutornenko wrote:
> > src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java, line 98
> > 
> >
> > It would make sense to handle read and write latencies separately to 
> > better simulate real-world perf.

Sounds good, added a TODO.


> On April 28, 2015, 8:35 p.m., Maxim Khutornenko wrote:
> > src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java, lines 
> > 105-110
> > 
> >
> > +1. System.exit() won't pass static analysis anyway.

Done.


> On April 28, 2015, 8:35 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/mesos/Driver.java, line 64
> > 
> >
> > Is this required for this diff? I don't see any callers.

Doesn't appear to be needed, removed it.


> On April 28, 2015, 8:35 p.m., Maxim Khutornenko wrote:
> > src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java, line 
> > 301
> > 
> >
> > dron newline

Done.


> On April 28, 2015, 8:35 p.m., Maxim Khutornenko wrote:
> > src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java, line 
> > 299
> > 
> >
> > This needs to be a "non-guessable" value. You can use something like 
> > `System.currentTimeMillis() % 5 == 0` instead. More here: 
> > http://hg.openjdk.java.net/code-tools/jmh/file/549b2aaf63ca/jmh-samples/src/main/java/org/openjdk/jmh/samples/JMHSample_09_Blackholes.java

Updated per your suggestion.


> On April 28, 2015, 8:35 p.m., Maxim Khutornenko wrote:
> > src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java, line 
> > 283
> > 
> >
> > `for (String taskId : Tasks.ids(tasks))`

Done.


> On April 28, 2015, 8:35 p.m., Maxim Khutornenko wrote:
> > src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java, line 
> > 260
> > 
> >
> > This will go away if you take my suggestion below.

Done.


> On April 28, 2015, 8:35 p.m., Maxim Khutornenko wrote:
> > src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java, line 
> > 210
> > 
> >
> > Can be inlined with anonymous implementation instead.

Done.


> On April 28, 2015, 8:35 p.m., Maxim Khutornenko wrote:
> > src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java, line 
> > 166
> > 
> >
> > This needs to be Level.Trial instead. The task creation part at the 
> > bottom can be moved into another method marked as Level.Invocation.

Done.


> On April 28, 2015, 8:35 p.m., Maxim Khutornenko wrote:
> > src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java, line 
> > 175
> > 
> >
> > Is this needed? If not you can get rid of the local variable and create 
> > it inline with binding.

Done.


> On April 28, 2015, 8:35 p.m., Maxim Khutornenko wrote:
> > src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java, lines 
> > 189-201
> > 
> >
> > How about creating a FakeOfferManager instead? That would let you get 
> > rid of this private module as well as ScheduledExecutorService binding.

Done.


> On April 28, 2015, 8:35 p.m., Maxim Khutornenko wrote:
> > src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java, line 
> > 274
> >

Re: Review Request 33608: Added a status update throughput benchmark.

2015-04-28 Thread Ben Mahler 

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

(Updated April 29, 2015, 12:48 a.m.)


Review request for Aurora and Maxim Khutornenko.


Changes
---

Review feedback.


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


Repository: aurora


Description
---

In order to justify doing asynchronous batch acknowledgements and to better 
understand status update throughput, this introduces a benchmark.

Note that this assumes that status update processing is not synchronous, so 
that the benchmark doesn't need to be updated for AURORA-1228.


Diffs (updated)
-

  src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java 
PRE-CREATION 
  src/jmh/java/org/apache/aurora/benchmark/fakes/FakeCommand.java PRE-CREATION 
  src/jmh/java/org/apache/aurora/benchmark/fakes/FakeOfferManager.java 
PRE-CREATION 
  src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 
c54619f7cd617b48069363173dcf63b6254b4095 
  src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java 
9b8ab7c1027731f9d3f6cae77b85272ea63354d4 
  src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 
d7d659bb13f085ff06291ef0033572f8bdf29874 

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


Testing
---

Ran the benchmarks against the existing code and some pending code I have for 
AURORA-1228 to demonstrate the improvement.

The end to end tests are broken, appears to be unrelated to my change from what 
I can tell.


Thanks,

Ben Mahler

Re: Review Request 33608: Added a status update throughput benchmark.

2015-04-28 Thread Ben Mahler 


> On April 28, 2015, 1:30 a.m., Kevin Sweeney wrote:
> > src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java, lines 
> > 105-110
> > 
> >
> > You probably want 
> > http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/util/concurrent/Uninterruptibles.html#sleepUninterruptibly(long,%20java.util.concurrent.TimeUnit)
> >  here.

Thanks!


- Ben


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


On April 28, 2015, 12:57 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33608/
> ---
> 
> (Updated April 28, 2015, 12:57 a.m.)
> 
> 
> Review request for Aurora and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1283
> https://issues.apache.org/jira/browse/AURORA-1283
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> In order to justify doing asynchronous batch acknowledgements and to better 
> understand status update throughput, this introduces a benchmark.
> 
> Note that this assumes that status update processing is not synchronous, so 
> that the benchmark doesn't need to be updated for AURORA-1228.
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeCommand.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeDriver.java 
> 45de15a57baf7a2f7d437b590935714e28777f35 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java 
> PRE-CREATION 
>   
> src/jmh/java/org/apache/aurora/benchmark/fakes/FakeUncaughtExceptionHandler.java
>  PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 
> c54619f7cd617b48069363173dcf63b6254b4095 
>   src/main/java/org/apache/aurora/scheduler/mesos/Driver.java 
> c7e45a89ceaa2c310feb610091eec0b04187860e 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java 
> 9b8ab7c1027731f9d3f6cae77b85272ea63354d4 
>   src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 
> d7d659bb13f085ff06291ef0033572f8bdf29874 
>   src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverService.java 
> da2d5df2e053e6e1b8fb08d6813dff9eac9777f8 
> 
> Diff: https://reviews.apache.org/r/33608/diff/
> 
> 
> Testing
> ---
> 
> Ran the benchmarks against the existing code and some pending code I have for 
> AURORA-1228 to demonstrate the improvement.
> 
> The end to end tests are broken, appears to be unrelated to my change from 
> what I can tell.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 33608: Added a status update throughput benchmark.

2015-04-28 Thread Maxim Khutornenko 

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



src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java


Please, add Apache headers to all new files.



src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java


s/public/private or move it into its own file.



src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java


It would make sense to handle read and write latencies separately to better 
simulate real-world perf.



src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java


+1. System.exit() won't pass static analysis anyway.



src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java


This needs to be Level.Trial instead. The task creation part at the bottom 
can be moved into another method marked as Level.Invocation.



src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java


Is this needed? If not you can get rid of the local variable and create it 
inline with binding.



src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java


How about creating a FakeOfferManager instead? That would let you get rid 
of this private module as well as ScheduledExecutorService binding.



src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java


Can be inlined with anonymous implementation instead.



src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java


This will go away if you take my suggestion below.



src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java


Does not need to be wrapped into Object. Just define the method inside the 
class and register it in setUp as `eventBus.register(this)`



src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java


`for (String taskId : Tasks.ids(tasks))`



src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java


This is not needed as you register it every time in setup.



src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java


This needs to be a "non-guessable" value. You can use something like 
`System.currentTimeMillis() % 5 == 0` instead. More here: 
http://hg.openjdk.java.net/code-tools/jmh/file/549b2aaf63ca/jmh-samples/src/main/java/org/openjdk/jmh/samples/JMHSample_09_Blackholes.java



src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java


dron newline



src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java


Please, run ./gradlew -Pq build to pick up style warnings and run static 
analysis tools.



src/main/java/org/apache/aurora/scheduler/mesos/Driver.java


Is this required for this diff? I don't see any callers.


- Maxim Khutornenko


On April 28, 2015, 12:57 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33608/
> ---
> 
> (Updated April 28, 2015, 12:57 a.m.)
> 
> 
> Review request for Aurora and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1283
> https://issues.apache.org/jira/browse/AURORA-1283
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> In order to justify doing asynchronous batch acknowledgements and to better 
> understand status update throughput, this introduces a benchmark.
> 
> Note that this assumes that status update processing is not synchronous, so 
> that the benchmark doesn't need to be updated for AURORA-1228.
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeCommand.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeDriver.java 
> 45de15a57baf7a2f7d437b590935714e28777f35 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java 
> PRE-CREATION 
>   
> src/jmh/java/org/apache/aurora/benchmark/fakes/FakeUncaughtExceptionHandler.java
>  PR

Re: Review Request 33608: Added a status update throughput benchmark.

2015-04-27 Thread Kevin Sweeney 

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



src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java


You probably want 
http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/util/concurrent/Uninterruptibles.html#sleepUninterruptibly(long,%20java.util.concurrent.TimeUnit)
 here.


- Kevin Sweeney


On April 27, 2015, 5:57 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33608/
> ---
> 
> (Updated April 27, 2015, 5:57 p.m.)
> 
> 
> Review request for Aurora and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1283
> https://issues.apache.org/jira/browse/AURORA-1283
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> In order to justify doing asynchronous batch acknowledgements and to better 
> understand status update throughput, this introduces a benchmark.
> 
> Note that this assumes that status update processing is not synchronous, so 
> that the benchmark doesn't need to be updated for AURORA-1228.
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeCommand.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeDriver.java 
> 45de15a57baf7a2f7d437b590935714e28777f35 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java 
> PRE-CREATION 
>   
> src/jmh/java/org/apache/aurora/benchmark/fakes/FakeUncaughtExceptionHandler.java
>  PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 
> c54619f7cd617b48069363173dcf63b6254b4095 
>   src/main/java/org/apache/aurora/scheduler/mesos/Driver.java 
> c7e45a89ceaa2c310feb610091eec0b04187860e 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java 
> 9b8ab7c1027731f9d3f6cae77b85272ea63354d4 
>   src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 
> d7d659bb13f085ff06291ef0033572f8bdf29874 
>   src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverService.java 
> da2d5df2e053e6e1b8fb08d6813dff9eac9777f8 
> 
> Diff: https://reviews.apache.org/r/33608/diff/
> 
> 
> Testing
> ---
> 
> Ran the benchmarks against the existing code and some pending code I have for 
> AURORA-1228 to demonstrate the improvement.
> 
> The end to end tests are broken, appears to be unrelated to my change from 
> what I can tell.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 33608: Added a status update throughput benchmark.

2015-04-27 Thread Aurora ReviewBot 

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


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

:compileJavaNote: Writing 
file:/home/jenkins/jenkins-slave/workspace/AuroraBot/dist/classes/main/com/twitter/common/args/apt/cmdline.arg.info.txt.2

:processResources
:classes
:jar
:startScripts
:distTar
:distZip
:assemble
:compileJmhJavaNote: 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java
 uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.

:processJmhResources UP-TO-DATE
:jmhClasses
:checkstyleJmh[ant:checkstyle] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java:1:
 Line does not match expected header line of '^\/\*\*$'.
[ant:checkstyle] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java:68:
 Wrong order for 'com.twitter.common.application.ShutdownStage' import. Order 
should be: java, javax, scala, com, net, org. Each group should be 
separated by a single blank line.
[ant:checkstyle] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java:109:
 Dont System.exit(), throw a RuntimeException()
[ant:checkstyle] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java:121:
 Dont System.exit(), throw a RuntimeException()
[ant:checkstyle] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java:134:
 Dont System.exit(), throw a RuntimeException()
[ant:checkstyle] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/jmh/java/org/apache/aurora/benchmark/fakes/FakeCommand.java:1:
 Missing a header - not enough lines in file.
[ant:checkstyle] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java:1:
 Line does not match expected header line of '^\/\*\*$'.
[ant:checkstyle] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java:45:
 Line is longer than 100 characters (found 132).
[ant:checkstyle] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java:50:
 Line is longer than 100 characters (found 108).
[ant:checkstyle] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java:55:
 Line is longer than 100 characters (found 119).
[ant:checkstyle] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java:90:
 Line is longer than 100 characters (found 112).
[ant:checkstyle] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/jmh/java/org/apache/aurora/benchmark/fakes/FakeUncaughtExceptionHandler.java:1:
 Missing a header - not enough lines in file.
 FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':checkstyleJmh'.
> Checkstyle rule violations were found. See the report at: 
> file:///home/jenkins/jenkins-slave/workspace/AuroraBot/dist/reports/checkstyle/jmh.xml

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug 
option to get more log output.

BUILD FAILED

Total time: 1 mins 42.956 secs


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

- Aurora ReviewBot


On April 28, 2015, 12:57 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33608/
> ---
> 
> (Updated April 28, 2015, 12:57 a.m.)
> 
> 
> Review request for Aurora and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1283
> https://issues.apache.org/jira/browse/AURORA-1283
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> In order to justify doing asynchronous batch acknowledgements and to better 
> understand status update throughput, this introduces a benchmark.
> 
> Note that this assumes that status update processing is not synchronous, so 
> that the benchmark doesn't need to be updated for AURORA-1228.
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeCommand.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeDriver.java 
> 45de15a57baf7a2f7d437b590935714e28777f35 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java 
> PRE-CREATION 
>

Review Request 33608: Added a status update throughput benchmark.

2015-04-27 Thread Ben Mahler 

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

Review request for Aurora and Maxim Khutornenko.


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


Repository: aurora


Description
---

In order to justify doing asynchronous batch acknowledgements and to better 
understand status update throughput, this introduces a benchmark.

Note that this assumes that status update processing is not synchronous, so 
that the benchmark doesn't need to be updated for AURORA-1228.


Diffs
-

  src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java 
PRE-CREATION 
  src/jmh/java/org/apache/aurora/benchmark/fakes/FakeCommand.java PRE-CREATION 
  src/jmh/java/org/apache/aurora/benchmark/fakes/FakeDriver.java 
45de15a57baf7a2f7d437b590935714e28777f35 
  src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java 
PRE-CREATION 
  
src/jmh/java/org/apache/aurora/benchmark/fakes/FakeUncaughtExceptionHandler.java
 PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 
c54619f7cd617b48069363173dcf63b6254b4095 
  src/main/java/org/apache/aurora/scheduler/mesos/Driver.java 
c7e45a89ceaa2c310feb610091eec0b04187860e 
  src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java 
9b8ab7c1027731f9d3f6cae77b85272ea63354d4 
  src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 
d7d659bb13f085ff06291ef0033572f8bdf29874 
  src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverService.java 
da2d5df2e053e6e1b8fb08d6813dff9eac9777f8 

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


Testing
---

Ran the benchmarks against the existing code and some pending code I have for 
AURORA-1228 to demonstrate the improvement.

The end to end tests are broken, appears to be unrelated to my change from what 
I can tell.


Thanks,

Ben Mahler