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


Fix it, then Ship it!




Lgtm. Two comments below.


src/main/java/org/apache/aurora/scheduler/state/TaskStateMachine.java (line 474)
<https://reviews.apache.org/r/54269/#comment228312>

    Please use the logging builtin formatting rather than  doing the string 
concatenate all the time.


Please mention the logger config change in the release notes.

- Stephan Erb


On Dez. 2, 2016, 12:13 vorm., Zameer Manji wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54269/
> -----------------------------------------------------------
> 
> (Updated Dez. 2, 2016, 12:13 vorm.)
> 
> 
> Review request for Aurora, David McLaughlin, Joshua Cohen, and Stephan Erb.
> 
> 
> Bugs: AURORA-1831
>     https://issues.apache.org/jira/browse/AURORA-1831
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> This patch makes two logging performance changes.
> 
> First, it reduces the cost of logging by replacing the costly class and line
> patterns with the cheaper logger pattern. We lose line numbers and inner class
> information for much cheaper logging.
> 
> Before
> ````
> I1201 15:08:40.560 [AsyncProcessor-0, StateMachine$Builder:389] 
> SchedulerLifecycle state machine transition LEADER_AWAITING_REGISTRATION -> 
> ACTIVE
> ````
> 
> After
> ````
> I1201 15:06:47.181 [AsyncProcessor-0, StateMachine] SchedulerLifecycle state 
> machine transition LEADER_AWAITING_REGISTRATION -> ACTIVE
> ````
> 
> Second, it reduces the verbosity of the `TaskStateMachine` logging where it 
> logs
> the work command from the transition. I don't think there is any operator 
> value
> in logging this (unlike the task state transitions) so I have lowered it to
> debug.
> 
> Performance Before:
> 
> ````
> Benchmark                                               (numPendingTasks)  
> (numTasksToDelete)   Mode  Cnt  Score   Error  Units
> StateManagerBenchmarks.DeleteTasksBenchmark.run                       N/A     
>            1000  thrpt   10  2.510 ± 0.557  ops/s
> StateManagerBenchmarks.DeleteTasksBenchmark.run                       N/A     
>           10000  thrpt   10  0.272 ± 0.030  ops/s
> StateManagerBenchmarks.DeleteTasksBenchmark.run                       N/A     
>           50000  thrpt   10  0.053 ± 0.011  ops/s
> StateManagerBenchmarks.InsertPendingTasksBenchmark.run               1000     
>             N/A  thrpt   10  2.446 ± 0.698  ops/s
> StateManagerBenchmarks.InsertPendingTasksBenchmark.run              10000     
>             N/A  thrpt   10  0.246 ± 0.018  ops/s
> StateManagerBenchmarks.InsertPendingTasksBenchmark.run              50000     
>             N/A  thrpt   10  0.041 ± 0.006  ops/s
> ````
> 
> Performance After:
> ````
> Benchmark                                               (numPendingTasks)  
> (numTasksToDelete)   Mode  Cnt   Score   Error  Units
> StateManagerBenchmarks.DeleteTasksBenchmark.run                       N/A     
>            1000  thrpt   10  14.520 ± 5.696  ops/s
> StateManagerBenchmarks.DeleteTasksBenchmark.run                       N/A     
>           10000  thrpt   10   1.290 ± 0.361  ops/s
> StateManagerBenchmarks.DeleteTasksBenchmark.run                       N/A     
>           50000  thrpt   10   0.254 ± 0.097  ops/s
> StateManagerBenchmarks.InsertPendingTasksBenchmark.run               1000     
>             N/A  thrpt    5   7.303 ± 5.662  ops/s
> StateManagerBenchmarks.InsertPendingTasksBenchmark.run              10000     
>             N/A  thrpt    5   0.726 ± 0.624  ops/s
> StateManagerBenchmarks.InsertPendingTasksBenchmark.run              50000     
>             N/A  thrpt    5   0.124 ± 0.058  ops/s
> ````
> 
> There is a performance improvement in the smaller case and no noticable
> degredation in the larger cases. I also verified on a small cluster that the
> improvements exist for the larger cases. I am unable to reduce the error bars
> locally on the `InsertPendingTasksBenchmark`. I suspect the bencmark needs to 
> be
> tweaked to be more consistent.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/state/TaskStateMachine.java 
> 23f256a7d467c79dcd5d906f76af4f0261dfd81d 
>   src/main/resources/logback.xml 84c175cec811fd95db44fd8dfcd514385606042d 
> 
> Diff: https://reviews.apache.org/r/54269/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Reply via email to