aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ben Mahler" <benjamin.mah...@gmail.com>
Subject Re: Review Request 33608: Added a status update throughput benchmark.
Date Wed, 29 Apr 2015 00:48:01 GMT


> 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
> > <https://reviews.apache.org/r/33608/diff/1/?file=943455#file943455line45>
> >
> >     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
> > <https://reviews.apache.org/r/33608/diff/1/?file=943452#file943452line1>
> >
> >     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
> > <https://reviews.apache.org/r/33608/diff/1/?file=943452#file943452line94>
> >
> >     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
> > <https://reviews.apache.org/r/33608/diff/1/?file=943452#file943452line98>
> >
> >     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
> > <https://reviews.apache.org/r/33608/diff/1/?file=943452#file943452line105>
> >
> >     +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
> > <https://reviews.apache.org/r/33608/diff/1/?file=943458#file943458line64>
> >
> >     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
> > <https://reviews.apache.org/r/33608/diff/1/?file=943452#file943452line301>
> >
> >     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
> > <https://reviews.apache.org/r/33608/diff/1/?file=943452#file943452line299>
> >
> >     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
> > <https://reviews.apache.org/r/33608/diff/1/?file=943452#file943452line283>
> >
> >     `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
> > <https://reviews.apache.org/r/33608/diff/1/?file=943452#file943452line260>
> >
> >     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
> > <https://reviews.apache.org/r/33608/diff/1/?file=943452#file943452line210>
> >
> >     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
> > <https://reviews.apache.org/r/33608/diff/1/?file=943452#file943452line166>
> >
> >     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
> > <https://reviews.apache.org/r/33608/diff/1/?file=943452#file943452line175>
> >
> >     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
> > <https://reviews.apache.org/r/33608/diff/1/?file=943452#file943452line189>
> >
> >     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
> > <https://reviews.apache.org/r/33608/diff/1/?file=943452#file943452line274>
> >
> >     Does not need to be wrapped into Object. Just define the method inside the class
and register it in setUp as `eventBus.register(this)`

Ok, it was to localize the latch, but FindBugs had a false positive for this being unreachable
code anyway. Done.


> On April 28, 2015, 8:35 p.m., Maxim Khutornenko wrote:
> > src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java, line 297
> > <https://reviews.apache.org/r/33608/diff/1/?file=943452#file943452line297>
> >
> >     This is not needed as you register it every time in setup.

It was needed for the Object case, but since I now register `this`, I've removed this.


- Ben


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


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


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