From 6e4b760f8340efd1b4dd0a40d9d3bffc35353d18 Mon Sep 17 00:00:00 2001 From: Zack Dever Date: Fri, 25 Mar 2016 15:00:53 -0700 Subject: [PATCH 1/2] change initial value of Min stat to Double.MAX_VALUE (not MIN) this is consistent with the Max stat implementation. also update variable names to `min` instead of `max`, and test. --- .../org/apache/kafka/common/metrics/stats/Min.java | 8 ++++---- .../org/apache/kafka/common/metrics/MetricsTest.java | 10 ++++++++++ 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/clients/src/main/java/org/apache/kafka/common/metrics/stats/Min.java b/clients/src/main/java/org/apache/kafka/common/metrics/stats/Min.java index 9f74417193a04..113d745c3b185 100644 --- a/clients/src/main/java/org/apache/kafka/common/metrics/stats/Min.java +++ b/clients/src/main/java/org/apache/kafka/common/metrics/stats/Min.java @@ -22,7 +22,7 @@ public class Min extends SampledStat { public Min() { - super(Double.MIN_VALUE); + super(Double.MAX_VALUE); } @Override @@ -32,10 +32,10 @@ protected void update(Sample sample, MetricConfig config, double value, long now @Override public double combine(List samples, MetricConfig config, long now) { - double max = Double.MAX_VALUE; + double min = Double.MAX_VALUE; for (int i = 0; i < samples.size(); i++) - max = Math.min(max, samples.get(i).value); - return max; + min = Math.min(min, samples.get(i).value); + return min; } } diff --git a/clients/src/test/java/org/apache/kafka/common/metrics/MetricsTest.java b/clients/src/test/java/org/apache/kafka/common/metrics/MetricsTest.java index f5b49ba55ca90..a6703476cbc60 100644 --- a/clients/src/test/java/org/apache/kafka/common/metrics/MetricsTest.java +++ b/clients/src/test/java/org/apache/kafka/common/metrics/MetricsTest.java @@ -308,12 +308,22 @@ public void testTimeWindowing() { @Test public void testOldDataHasNoEffect() { Max max = new Max(); + Min min = new Min(); + Avg avg = new Avg(); + Count count = new Count(); long windowMs = 100; int samples = 2; MetricConfig config = new MetricConfig().timeWindow(windowMs, TimeUnit.MILLISECONDS).samples(samples); max.record(config, 50, time.milliseconds()); + min.record(config, 50, time.milliseconds()); + avg.record(config, 50, time.milliseconds()); + count.record(config, 50, time.milliseconds()); time.sleep(samples * windowMs); + // this also serves as a check for a sane initialValue on each Stat assertEquals(Double.NEGATIVE_INFINITY, max.measure(config, time.milliseconds()), EPS); + assertEquals(Double.MAX_VALUE, min.measure(config, time.milliseconds()), EPS); + assertEquals(0.0, avg.measure(config, time.milliseconds()), EPS); + assertEquals(0, count.measure(config, time.milliseconds()), EPS); } @Test(expected = IllegalArgumentException.class) From f3c7403338b0c29b2e09a39975d01d3a62722d18 Mon Sep 17 00:00:00 2001 From: Zack Dever Date: Thu, 14 Apr 2016 16:45:17 -0700 Subject: [PATCH 2/2] create new testSampledStatInitialValue instead of repurposing testOldDataHasNoEffect --- .../kafka/common/metrics/MetricsTest.java | 24 ++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/clients/src/test/java/org/apache/kafka/common/metrics/MetricsTest.java b/clients/src/test/java/org/apache/kafka/common/metrics/MetricsTest.java index a6703476cbc60..52f0cd7e04bec 100644 --- a/clients/src/test/java/org/apache/kafka/common/metrics/MetricsTest.java +++ b/clients/src/test/java/org/apache/kafka/common/metrics/MetricsTest.java @@ -307,10 +307,30 @@ public void testTimeWindowing() { @Test public void testOldDataHasNoEffect() { + Max max = new Max(); + long windowMs = 100; + int samples = 2; + MetricConfig config = new MetricConfig().timeWindow(windowMs, TimeUnit.MILLISECONDS).samples(samples); + max.record(config, 50, time.milliseconds()); + time.sleep(samples * windowMs); + assertEquals(Double.NEGATIVE_INFINITY, max.measure(config, time.milliseconds()), EPS); + } + + @Test + public void testSampledStatInitialValue() { + // initialValue from each SampledStat is set as the initialValue on its Sample. + // The only way to test the initialValue is to infer it by having a SampledStat + // with expired Stats, because their values are reset to the initial values. + // Most implementations of combine on SampledStat end up returning the default + // value, so we can use this. This doesn't work for Percentiles though. + // This test looks a lot like testOldDataHasNoEffect because it's the same + // flow that leads to this state. Max max = new Max(); Min min = new Min(); Avg avg = new Avg(); Count count = new Count(); + Rate.SampledTotal sampledTotal = new Rate.SampledTotal(); + long windowMs = 100; int samples = 2; MetricConfig config = new MetricConfig().timeWindow(windowMs, TimeUnit.MILLISECONDS).samples(samples); @@ -318,12 +338,14 @@ public void testOldDataHasNoEffect() { min.record(config, 50, time.milliseconds()); avg.record(config, 50, time.milliseconds()); count.record(config, 50, time.milliseconds()); + sampledTotal.record(config, 50, time.milliseconds()); time.sleep(samples * windowMs); - // this also serves as a check for a sane initialValue on each Stat + assertEquals(Double.NEGATIVE_INFINITY, max.measure(config, time.milliseconds()), EPS); assertEquals(Double.MAX_VALUE, min.measure(config, time.milliseconds()), EPS); assertEquals(0.0, avg.measure(config, time.milliseconds()), EPS); assertEquals(0, count.measure(config, time.milliseconds()), EPS); + assertEquals(0.0, sampledTotal.measure(config, time.milliseconds()), EPS); } @Test(expected = IllegalArgumentException.class)