----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53687/#review155959 -----------------------------------------------------------
+1 to Zameer's feedback as well, especially w.r.t. to tests. commons/src/main/java/org/apache/aurora/common/stats/SlidingStats.java (line 31) <https://reviews.apache.org/r/53687/#comment226048> One general goal is to use the minimum visibility necessary to get things done. I don't see `perEventLatency` referenced from the new subclass, so in theory it should be able to stay `private`? commons/src/main/java/org/apache/aurora/common/stats/SlidingTimedStats.java (line 33) <https://reviews.apache.org/r/53687/#comment226043> s/name/name. commons/src/main/java/org/apache/aurora/common/stats/SlidingTimedStats.java (lines 39 - 40) <https://reviews.apache.org/r/53687/#comment226042> indentation is off here it should be: this( name, totalUnitDisplay, DEFAULT_WINDOW_SIZE, new Percentile<Long>(...)); (See https://github.com/twitter/commons/blob/master/src/java/com/twitter/common/styleguide.md for the guidelines, most are enforced by checkstyle, but some unfortunately are not/can not be) commons/src/main/java/org/apache/aurora/common/stats/SlidingTimedStats.java (line 44) <https://reviews.apache.org/r/53687/#comment226044> s/name/name. commons/src/main/java/org/apache/aurora/common/stats/SlidingTimedStats.java (line 53) <https://reviews.apache.org/r/53687/#comment226046> s/name ,/name, commons/src/main/java/org/apache/aurora/common/stats/SlidingTimedStats.java (lines 66 - 67) <https://reviews.apache.org/r/53687/#comment226047> Should this just be `super.accumulate(value)`? That would avoid needing to change the visisibility of `total` and `events` commons/src/main/java/org/apache/aurora/common/stats/SlidingTimedStats.java (line 69) <https://reviews.apache.org/r/53687/#comment226045> formatting should be: if (percentile != null) { percentile.record(value); } That said, we try and avoid the need for `!= null` checks by using `Optional` instead. Of course, this is all dependent on whether or not this check is necessary to begin with, as I agree with Zameer, the reason for making this optional is non-obvious, so perhaps it can just go away completely? - Joshua Cohen On Nov. 12, 2016, 1:49 a.m., Reza Motamedi wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53687/ > ----------------------------------------------------------- > > (Updated Nov. 12, 2016, 1:49 a.m.) > > > Review request for Aurora and Joshua Cohen. > > > Repository: aurora > > > Description > ------- > > track percentile values for timed sliding stats. AURORA-118 > > > Diffs > ----- > > commons/src/main/java/org/apache/aurora/common/inject/TimedInterceptor.java > a4c4240b67cafb108ca587ad86d9fb9d9bb1553d > commons/src/main/java/org/apache/aurora/common/stats/SlidingStats.java > f7a5ae41e307627fc55157758e9b7cdd861c3268 > commons/src/main/java/org/apache/aurora/common/stats/SlidingTimedStats.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/53687/diff/ > > > Testing > ------- > > This is a patch for the request in > https://issues.apache.org/jira/browse/AURORA-118. > > SlidingTimedStats extends SlidingStats and contains aPercentile for tracking > a finer view of the timing stats. Percentile was already implemented to track > RequestStats and I reused it to track any timing stats captured via @Timed > annotation. > > > Thanks, > > Reza Motamedi > >
