Mailing-List: contact dev-help@samza.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@samza.apache.org
Content-Type: multipart/alternative;
 boundary="===============1840225662274039954=="
MIME-Version: 1.0
Subject: Re: Review Request 37069: SAMZA-738 Samza Timer based metrics does
 not have enough precision
From: "Yi Pan (Data Infrastructure)" <yipan@linkedin.com>
To: "Aleksandar Pejakovic" <a.pejakovic@levi9.com>,
 "samza" <dev@samza.apache.org>,
 "Yi Pan (Data Infrastructure)" <yipan@linkedin.com>
Date: Tue, 04 Aug 2015 22:36:10 -0000
Message-ID: <20150804223610.17264.21964@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Yi Pan (Data Infrastructure)" <noreply@reviews.apache.org>
References: <20150804091849.4206.40365@reviews.apache.org>
In-Reply-To: <20150804091849.4206.40365@reviews.apache.org>
Reply-To: "Yi Pan (Data Infrastructure)" <yipan@linkedin.com>

--===============1840225662274039954==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


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


samza-core/src/main/scala/org/apache/samza/container/RunLoop.scala (line 46)
<https://reviews.apache.org/r/37069/#comment148638>

    Why are we adding a new clock here? changing clock() implementation to:
    val clock: () => Long = {System.nanoTime}
    
    Should work as well.


samza-core/src/main/scala/org/apache/samza/container/RunLoop.scala (line 49)
<https://reviews.apache.org/r/37069/#comment148634>

    The variable should be lastWindowNs and lastCommitNs


samza-core/src/main/scala/org/apache/samza/container/RunLoop.scala (line 78)
<https://reviews.apache.org/r/37069/#comment148635>

    This should be totalNs


samza-core/src/main/scala/org/apache/samza/container/RunLoop.scala (line 107)
<https://reviews.apache.org/r/37069/#comment148636>

    This variable name should also be activeNs


samza-core/src/main/scala/org/apache/samza/container/RunLoop.scala (line 134)
<https://reviews.apache.org/r/37069/#comment148637>

    Same here.


samza-core/src/main/scala/org/apache/samza/container/RunLoop.scala (line 135)
<https://reviews.apache.org/r/37069/#comment148641>

    This does not need to be done by metricClocks. The original code works. Just that there is optimization we should do here to reduce the number of clock() calls. updateTimerAndGetDuration internally calls clock(), and we are calling clock() twice here as well. Should at least cache the return value from clock() and re-use it here.


- Yi Pan (Data Infrastructure)


On Aug. 4, 2015, 9:18 a.m., Aleksandar Pejakovic wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37069/
> -----------------------------------------------------------
> 
> (Updated Aug. 4, 2015, 9:18 a.m.)
> 
> 
> Review request for samza.
> 
> 
> Repository: samza
> 
> 
> Description
> -------
> 
> Changed SystemProducersMetrics and RunLoop so that metrics now show nanoseconds instead milliseconds.
> 
> 
> Diffs
> -----
> 
>   samza-core/src/main/scala/org/apache/samza/container/RunLoop.scala c292ae4 
>   samza-core/src/main/scala/org/apache/samza/container/SamzaContainerMetrics.scala aa7a9bc 
> 
> Diff: https://reviews.apache.org/r/37069/diff/
> 
> 
> Testing
> -------
> 
> Tested on hello-samza -> wikipedia-parser, results:
> ```
> "org.apache.samza.container.SamzaContainerMetrics":{
>   "commit-calls":10,
>   "window-ns":3198.62544796632,
>   "process-null-envelopes":56292,
>   "process-envelopes":989,
>   "window-calls":0,
>   "commit-ns":5130.901534393375,
>   "send-calls":0,
>   "process-calls":57283,
>   "choose-ns":10368839.818551894,
>   "process-ns":10390588.194071393,
>   "event-loop-utilization":0.99807554
> }
> ```
> 
> 
> Thanks,
> 
> Aleksandar Pejakovic
> 
>


--===============1840225662274039954==--