You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@samza.apache.org by Chris Riccomini <cr...@apache.org> on 2014/08/01 23:44:30 UTC
Re: Review Request 24141: SAMZA-349
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24141/#review49384
-----------------------------------------------------------
This looks really good. A few minor nits, but nothing major.
Visually, this looks thread safe to me. Given that it's so close to drop wizard's metrics, I think we should be fine.
samza-api/src/main/java/org/apache/samza/metrics/Clock.java
<https://reviews.apache.org/r/24141/#comment86416>
We already have:
./samza-api/src/main/java/org/apache/samza/util/Clock.java
I always just create an anonymous class with System.currentTimeMillis when I use it.
samza-api/src/main/java/org/apache/samza/metrics/SlidingTimeWindowReservoir.java
<https://reviews.apache.org/r/24141/#comment86422>
Add a time unit to this variable. _MS?
samza-api/src/main/java/org/apache/samza/metrics/SlidingTimeWindowReservoir.java
<https://reviews.apache.org/r/24141/#comment86424>
Would be nice to have a third constructor which has windowMs, but default clock, so you can change windowMs easily.
- Chris Riccomini
On July 31, 2014, 9:31 a.m., Yan Fang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24141/
> -----------------------------------------------------------
>
> (Updated July 31, 2014, 9:31 a.m.)
>
>
> Review request for samza.
>
>
> Bugs: SAMZA-349
> https://issues.apache.org/jira/browse/SAMZA-349
>
>
> Repository: samza
>
>
> Description
> -------
>
> Add a Timer metric.
>
> 1) Created Timer.class that uses Reservoir.class to store time durations.
> 2) Created Reservoir interface, which is to store different values.
> 3) Created SlidingTimeWindowReservoir. It removes values that are out of date. Use this as the default reservoir for the Timer.
> 4) Created Snapshot class to report the max/min/average in Reservoir.
> 5) Updated MetricsVisitor by adding Timer
> 6) Updated relevant classes that uses MetricsVisitor: JmxReporter.scala, MetricsSnapshotReporter.scala and ApplicationMasterRestServlet.scala
> 7) Unit tests
>
>
> Diffs
> -----
>
> build.gradle 3ad5fe3
> samza-api/src/main/java/org/apache/samza/metrics/Clock.java PRE-CREATION
> samza-api/src/main/java/org/apache/samza/metrics/MetricsVisitor.java f4f756a
> samza-api/src/main/java/org/apache/samza/metrics/Reservoir.java PRE-CREATION
> samza-api/src/main/java/org/apache/samza/metrics/SlidingTimeWindowReservoir.java PRE-CREATION
> samza-api/src/main/java/org/apache/samza/metrics/Snapshot.java PRE-CREATION
> samza-api/src/main/java/org/apache/samza/metrics/Timer.java PRE-CREATION
> samza-api/src/test/java/org/apache/samza/metrics/TestSlidingTimeWindowReservoir.java PRE-CREATION
> samza-api/src/test/java/org/apache/samza/metrics/TestSnapshot.java PRE-CREATION
> samza-api/src/test/java/org/apache/samza/metrics/TestTimer.java PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/metrics/reporter/JmxReporter.scala 8814e68
> samza-core/src/main/scala/org/apache/samza/metrics/reporter/MetricsSnapshotReporter.scala 9a56754
> samza-yarn/src/main/scala/org/apache/samza/webapp/ApplicationMasterRestServlet.scala d10dc38
>
> Diff: https://reviews.apache.org/r/24141/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Yan Fang
>
>