----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34011/#review83663 -----------------------------------------------------------
samza-graphite/src/main/java/org/apache/samza/metrics/reporter/GraphiteSnapshot.java <https://reviews.apache.org/r/34011/#comment134670> to be consistent, lets use the getMax() method from samzaSnapshot. I looked at the code briefly, did not see the error. Maybe providing a reproducable case will be helpful. Of course, feel free to correct the bug in samzaSnapshot if you figure it out. samza-graphite/src/main/java/org/apache/samza/metrics/reporter/SamzaGraphiteReporter.java <https://reviews.apache.org/r/34011/#comment134672> are those used somewhere? samza-graphite/src/main/java/org/apache/samza/metrics/reporter/SamzaGraphiteReporter.java <https://reviews.apache.org/r/34011/#comment134674> to be more flexible, in Samza, we will use metrics.reporter.%s.host, metrics.reporter.%s.port, etc. For example, we use metrics.reporter.reporter-name.stream for the kafka metrics. See http://samza.apache.org/learn/documentation/0.9/jobs/configuration-table.html It allows users to have different names for different graphiter servers. (if they have more than one). Also helps to keep flexisiblity and consistency in the configuration. You can use SamzaGraphiteReporter(String name, Config config, String containerName), where the name is for %s samza-graphite/src/main/java/org/apache/samza/metrics/reporter/SamzaGraphiteReporter.java <https://reviews.apache.org/r/34011/#comment134671> where is this used? samza-graphite/src/main/java/org/apache/samza/metrics/reporter/SamzaGraphiteReporter.java <https://reviews.apache.org/r/34011/#comment134677> when we register, we are using the name "getGraphiteMetricName", should it be the same here? Also, if you can add the doc for http://samza.apache.org/learn/documentation/0.9/container/metrics.html and http://samza.apache.org/learn/documentation/0.9/jobs/configuration-table.html , that will be great. Thanks! - Yan Fang On May 11, 2015, 10:10 a.m., Luis De Pombo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34011/ > ----------------------------------------------------------- > > (Updated May 11, 2015, 10:10 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-graphite/src/main/java/org/apache/samza/metrics/reporter/GraphiteCounter.java > PRE-CREATION > > samza-graphite/src/main/java/org/apache/samza/metrics/reporter/GraphiteGauge.java > PRE-CREATION > > samza-graphite/src/main/java/org/apache/samza/metrics/reporter/GraphiteReporterFactory.java > PRE-CREATION > > samza-graphite/src/main/java/org/apache/samza/metrics/reporter/GraphiteSnapshot.java > PRE-CREATION > > samza-graphite/src/main/java/org/apache/samza/metrics/reporter/GraphiteTimer.java > PRE-CREATION > > samza-graphite/src/main/java/org/apache/samza/metrics/reporter/SamzaGraphiteReporter.java > PRE-CREATION > > samza-graphite/src/test/java/org/apache/samza/metrics/reporter/GraphiteCounterTest.java > PRE-CREATION > > samza-graphite/src/test/java/org/apache/samza/metrics/reporter/GraphiteGaugeTest.java > PRE-CREATION > > samza-graphite/src/test/java/org/apache/samza/metrics/reporter/GraphiteSnapshotTest.java > PRE-CREATION > > samza-graphite/src/test/java/org/apache/samza/metrics/reporter/GraphiteTimerTest.java > PRE-CREATION > > samza-graphite/src/test/java/org/apache/samza/metrics/reporter/SamzaGraphiteReporterTest.java > PRE-CREATION > settings.gradle bb07a3b84b14dcef94da1bb166eab6aa3d0026bb > > Diff: https://reviews.apache.org/r/34011/diff/ > > > Testing > ------- > > > Thanks, > > Luis De Pombo > >
