-----------------------------------------------------------
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
<https://reviews.apache.org/r/33608/#comment132345>

    Please, add Apache headers to all new files.



src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java
<https://reviews.apache.org/r/33608/#comment132355>

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



src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java
<https://reviews.apache.org/r/33608/#comment132343>

    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
<https://reviews.apache.org/r/33608/#comment132354>

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



src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java
<https://reviews.apache.org/r/33608/#comment132381>

    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
<https://reviews.apache.org/r/33608/#comment132360>

    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
<https://reviews.apache.org/r/33608/#comment132342>

    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
<https://reviews.apache.org/r/33608/#comment132384>

    Can be inlined with anonymous implementation instead.



src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java
<https://reviews.apache.org/r/33608/#comment132366>

    This will go away if you take my suggestion below.



src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java
<https://reviews.apache.org/r/33608/#comment132369>

    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
<https://reviews.apache.org/r/33608/#comment132365>

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



src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java
<https://reviews.apache.org/r/33608/#comment132370>

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



src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java
<https://reviews.apache.org/r/33608/#comment132382>

    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
<https://reviews.apache.org/r/33608/#comment132383>

    dron newline



src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java
<https://reviews.apache.org/r/33608/#comment132346>

    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
<https://reviews.apache.org/r/33608/#comment132386>

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

Reply via email to