----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34011/#review83169 -----------------------------------------------------------
build.gradle <https://reviews.apache.org/r/34011/#comment134045> This shouldn't be in Samza-core. Create a separate module/project called samza-graphite (just like samza-log4j / samza-kv) gradle/dependency-versions.gradle <https://reviews.apache.org/r/34011/#comment134046> This is kinda surprising. We already have a metricsVersion defined ? I don't see it being used. samza-core/src/main/java/org/apache/samza/metrics/reporter/GraphiteCounter.java <https://reviews.apache.org/r/34011/#comment134047> Please move your changes to a separate module (not in samza-core) samza-core/src/main/java/org/apache/samza/metrics/reporter/GraphiteReporterFactory.java <https://reviews.apache.org/r/34011/#comment134049> I think lets pass the container name to the Samza graphite reporter factory. Might be useful if want to somehow aggregate metrics across all tasks in a container in the future. samza-core/src/main/java/org/apache/samza/metrics/reporter/GraphiteSnapshot.java <https://reviews.apache.org/r/34011/#comment134050> Add unit for value Make this static private ? samza-core/src/main/java/org/apache/samza/metrics/reporter/GraphiteSnapshot.java <https://reviews.apache.org/r/34011/#comment134052> Why not TimeUnit.MILLISECONDS.toNanos(value) ? The 'value' is in milliseconds right ? samza-core/src/main/java/org/apache/samza/metrics/reporter/GraphiteSnapshot.java <https://reviews.apache.org/r/34011/#comment134051> Make this static private ? samza-core/src/main/java/org/apache/samza/metrics/reporter/GraphiteSnapshot.java <https://reviews.apache.org/r/34011/#comment134053> Make it private ? samza-core/src/main/java/org/apache/samza/metrics/reporter/GraphiteSnapshot.java <https://reviews.apache.org/r/34011/#comment134054> Samza's Snapshot has a getMax samza-core/src/main/java/org/apache/samza/metrics/reporter/GraphiteSnapshot.java <https://reviews.apache.org/r/34011/#comment134056> Make all custom methods as private samza-core/src/main/java/org/apache/samza/metrics/reporter/GraphiteSnapshot.java <https://reviews.apache.org/r/34011/#comment134057> Throw an exception ? samza-core/src/main/java/org/apache/samza/metrics/reporter/GraphiteTimer.java <https://reviews.apache.org/r/34011/#comment134058> Check if duration >= 0L samza-core/src/main/java/org/apache/samza/metrics/reporter/GraphiteTimer.java <https://reviews.apache.org/r/34011/#comment134059> This will create a new object in each invocation. Why not store this as a member variable ? samza-core/src/main/java/org/apache/samza/metrics/reporter/SamzaGraphiteReporter.java <https://reviews.apache.org/r/34011/#comment134078> We should make this similar to the way metric names are constructed in other reporter (for instance check JmxReporter) samza-core/src/main/java/org/apache/samza/metrics/reporter/SamzaGraphiteReporter.java <https://reviews.apache.org/r/34011/#comment134079> Make it private samza-core/src/main/java/org/apache/samza/metrics/reporter/SamzaGraphiteReporter.java <https://reviews.apache.org/r/34011/#comment134080> Make it private static samza-core/src/main/java/org/apache/samza/metrics/reporter/SamzaGraphiteReporter.java <https://reviews.apache.org/r/34011/#comment134081> Use this format: getLong (name, default_value) samza-core/src/main/java/org/apache/samza/metrics/reporter/SamzaGraphiteReporter.java <https://reviews.apache.org/r/34011/#comment134082> Include the source as well (look at JmxReporter) samza-core/src/main/java/org/apache/samza/metrics/reporter/SamzaGraphiteReporter.java <https://reviews.apache.org/r/34011/#comment134083> Include the source as well (look at JmxReporter) for this and other metric names - Chinmay Soman On May 9, 2015, 6:31 a.m., Luis De Pombo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34011/ > ----------------------------------------------------------- > > (Updated May 9, 2015, 6:31 a.m.) > > > Review request for samza. > > > Repository: samza > > > Description > ------- > > Add support for a Graphite Metrics Reporter > > > Diffs > ----- > > build.gradle ac80a8664180e556ec83e229e04e3d8c56b70506 > checkstyle/import-control.xml 5f8e103a2e43f96518b20de1c7cbd84e0af24842 > gradle/dependency-versions.gradle ee6dfc411b7ab90b187df79f109884127953862e > > samza-core/src/main/java/org/apache/samza/metrics/reporter/GraphiteCounter.java > PRE-CREATION > > samza-core/src/main/java/org/apache/samza/metrics/reporter/GraphiteGauge.java > PRE-CREATION > > samza-core/src/main/java/org/apache/samza/metrics/reporter/GraphiteReporterFactory.java > PRE-CREATION > > samza-core/src/main/java/org/apache/samza/metrics/reporter/GraphiteSnapshot.java > PRE-CREATION > > samza-core/src/main/java/org/apache/samza/metrics/reporter/GraphiteTimer.java > PRE-CREATION > > samza-core/src/main/java/org/apache/samza/metrics/reporter/SamzaGraphiteReporter.java > PRE-CREATION > > samza-core/src/test/java/org/apache/samza/metrics/reporter/GraphiteCounterTest.java > PRE-CREATION > > samza-core/src/test/java/org/apache/samza/metrics/reporter/GraphiteGaugeTest.java > PRE-CREATION > > samza-core/src/test/java/org/apache/samza/metrics/reporter/GraphiteSnapshotTest.java > PRE-CREATION > > samza-core/src/test/java/org/apache/samza/metrics/reporter/GraphiteTimerTest.java > PRE-CREATION > > samza-core/src/test/java/org/apache/samza/metrics/reporter/SamzaGraphiteReporterTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/34011/diff/ > > > Testing > ------- > > > Thanks, > > Luis De Pombo > >
