aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joshua Cohen <jco...@apache.org>
Subject Re: Review Request 53687: track percentile values for timed sliding stats. AURORA-118
Date Tue, 15 Nov 2016 19:53:37 GMT

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


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