You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@flink.apache.org by nk...@apache.org on 2019/08/15 23:02:40 UTC
[flink] branch master updated: [FLINK-12987][metrics] fix
DescriptiveStatisticsHistogram#getCount() not returning the number of
elements seen
This is an automated email from the ASF dual-hosted git repository.
nkruber pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/flink.git
The following commit(s) were added to refs/heads/master by this push:
new fd9ef60 [FLINK-12987][metrics] fix DescriptiveStatisticsHistogram#getCount() not returning the number of elements seen
fd9ef60 is described below
commit fd9ef60cc8448a5f4d1915973e168aad073d8e8d
Author: Nico Kruber <ni...@ververica.com>
AuthorDate: Tue Jun 25 23:15:11 2019 +0200
[FLINK-12987][metrics] fix DescriptiveStatisticsHistogram#getCount() not returning the number of elements seen
DescriptiveStatisticsHistogram#getCount() only returned the number of currently
stored elements, not the total number seen.
Also add a unit test for verifying any Histogram implementation (based on
the previous test from DropwizardFlinkHistogramWrapperTest) and run this with
DescriptiveStatisticsHistogram.
---
flink-metrics/flink-metrics-core/pom.xml | 9 ++++
.../flink/metrics/AbstractHistogramTest.java | 60 ++++++++++++++++++++++
.../DropwizardFlinkHistogramWrapperTest.java | 27 ++--------
.../metrics/DescriptiveStatisticsHistogram.java | 5 +-
.../DescriptiveStatisticsHistogramTest.java | 40 +++++++++++++++
5 files changed, 116 insertions(+), 25 deletions(-)
diff --git a/flink-metrics/flink-metrics-core/pom.xml b/flink-metrics/flink-metrics-core/pom.xml
index f8bd768..524b3a2 100644
--- a/flink-metrics/flink-metrics-core/pom.xml
+++ b/flink-metrics/flink-metrics-core/pom.xml
@@ -34,6 +34,15 @@ under the License.
<packaging>jar</packaging>
+ <dependencies>
+ <!-- ================== test dependencies ================== -->
+
+ <dependency>
+ <groupId>org.apache.flink</groupId>
+ <artifactId>flink-test-utils-junit</artifactId>
+ </dependency>
+ </dependencies>
+
<build>
<plugins>
<!-- activate API compatibility checks -->
diff --git a/flink-metrics/flink-metrics-core/src/test/java/org/apache/flink/metrics/AbstractHistogramTest.java b/flink-metrics/flink-metrics-core/src/test/java/org/apache/flink/metrics/AbstractHistogramTest.java
new file mode 100644
index 0000000..d526b60c
--- /dev/null
+++ b/flink-metrics/flink-metrics-core/src/test/java/org/apache/flink/metrics/AbstractHistogramTest.java
@@ -0,0 +1,60 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.metrics;
+
+import org.apache.flink.util.TestLogger;
+
+import static org.junit.Assert.assertEquals;
+
+/**
+ * Abstract base class for testing {@link Histogram} and {@link HistogramStatistics} implementations.
+ */
+public class AbstractHistogramTest extends TestLogger {
+ protected void testHistogram(int size, Histogram histogram) {
+ HistogramStatistics statistics;
+
+ for (int i = 0; i < size; i++) {
+ histogram.update(i);
+
+ statistics = histogram.getStatistics();
+ assertEquals(i + 1, histogram.getCount());
+ assertEquals(histogram.getCount(), statistics.size());
+ assertEquals(i, statistics.getMax());
+ assertEquals(0, statistics.getMin());
+ }
+
+ statistics = histogram.getStatistics();
+ assertEquals(size, statistics.size());
+ assertEquals((size - 1) / 2.0, statistics.getQuantile(0.5), 0.001);
+
+ for (int i = size; i < 2 * size; i++) {
+ histogram.update(i);
+
+ statistics = histogram.getStatistics();
+ assertEquals(i + 1, histogram.getCount());
+ assertEquals(size, statistics.size());
+ assertEquals(i, statistics.getMax());
+ assertEquals(i + 1 - size, statistics.getMin());
+ }
+
+ statistics = histogram.getStatistics();
+ assertEquals(size, statistics.size());
+ assertEquals(size + (size - 1) / 2.0, statistics.getQuantile(0.5), 0.001);
+ }
+}
diff --git a/flink-metrics/flink-metrics-dropwizard/src/test/java/org/apache/flink/dropwizard/metrics/DropwizardFlinkHistogramWrapperTest.java b/flink-metrics/flink-metrics-dropwizard/src/test/java/org/apache/flink/dropwizard/metrics/DropwizardFlinkHistogramWrapperTest.java
index feec0f8..874636b 100644
--- a/flink-metrics/flink-metrics-dropwizard/src/test/java/org/apache/flink/dropwizard/metrics/DropwizardFlinkHistogramWrapperTest.java
+++ b/flink-metrics/flink-metrics-dropwizard/src/test/java/org/apache/flink/dropwizard/metrics/DropwizardFlinkHistogramWrapperTest.java
@@ -20,13 +20,13 @@ package org.apache.flink.dropwizard.metrics;
import org.apache.flink.configuration.ConfigConstants;
import org.apache.flink.dropwizard.ScheduledDropwizardReporter;
+import org.apache.flink.metrics.AbstractHistogramTest;
import org.apache.flink.metrics.MetricConfig;
import org.apache.flink.metrics.reporter.MetricReporter;
import org.apache.flink.runtime.metrics.MetricRegistryConfiguration;
import org.apache.flink.runtime.metrics.MetricRegistryImpl;
import org.apache.flink.runtime.metrics.ReporterSetup;
import org.apache.flink.runtime.metrics.groups.TaskManagerMetricGroup;
-import org.apache.flink.util.TestLogger;
import com.codahale.metrics.Counter;
import com.codahale.metrics.Gauge;
@@ -56,7 +56,7 @@ import static org.junit.Assert.assertTrue;
/**
* Tests for the DropwizardFlinkHistogramWrapper.
*/
-public class DropwizardFlinkHistogramWrapperTest extends TestLogger {
+public class DropwizardFlinkHistogramWrapperTest extends AbstractHistogramTest {
/**
* Tests the histogram functionality of the DropwizardHistogramWrapper.
@@ -66,28 +66,7 @@ public class DropwizardFlinkHistogramWrapperTest extends TestLogger {
int size = 10;
DropwizardHistogramWrapper histogramWrapper = new DropwizardHistogramWrapper(
new com.codahale.metrics.Histogram(new SlidingWindowReservoir(size)));
-
- for (int i = 0; i < size; i++) {
- histogramWrapper.update(i);
-
- assertEquals(i + 1, histogramWrapper.getCount());
- assertEquals(i, histogramWrapper.getStatistics().getMax());
- assertEquals(0, histogramWrapper.getStatistics().getMin());
- }
-
- assertEquals(size, histogramWrapper.getStatistics().size());
- assertEquals((size - 1) / 2.0, histogramWrapper.getStatistics().getQuantile(0.5), 0.001);
-
- for (int i = size; i < 2 * size; i++) {
- histogramWrapper.update(i);
-
- assertEquals(i + 1, histogramWrapper.getCount());
- assertEquals(i, histogramWrapper.getStatistics().getMax());
- assertEquals(i + 1 - size, histogramWrapper.getStatistics().getMin());
- }
-
- assertEquals(size, histogramWrapper.getStatistics().size());
- assertEquals(size + (size - 1) / 2.0, histogramWrapper.getStatistics().getQuantile(0.5), 0.001);
+ testHistogram(size, histogramWrapper);
}
/**
diff --git a/flink-runtime/src/main/java/org/apache/flink/runtime/metrics/DescriptiveStatisticsHistogram.java b/flink-runtime/src/main/java/org/apache/flink/runtime/metrics/DescriptiveStatisticsHistogram.java
index f01b984..9e99d14 100644
--- a/flink-runtime/src/main/java/org/apache/flink/runtime/metrics/DescriptiveStatisticsHistogram.java
+++ b/flink-runtime/src/main/java/org/apache/flink/runtime/metrics/DescriptiveStatisticsHistogram.java
@@ -29,18 +29,21 @@ public class DescriptiveStatisticsHistogram implements org.apache.flink.metrics.
private final DescriptiveStatistics descriptiveStatistics;
+ private long elementsSeen = 0L;
+
public DescriptiveStatisticsHistogram(int windowSize) {
this.descriptiveStatistics = new DescriptiveStatistics(windowSize);
}
@Override
public void update(long value) {
+ elementsSeen += 1L;
this.descriptiveStatistics.addValue(value);
}
@Override
public long getCount() {
- return this.descriptiveStatistics.getN();
+ return this.elementsSeen;
}
@Override
diff --git a/flink-runtime/src/test/java/org/apache/flink/runtime/metrics/DescriptiveStatisticsHistogramTest.java b/flink-runtime/src/test/java/org/apache/flink/runtime/metrics/DescriptiveStatisticsHistogramTest.java
new file mode 100644
index 0000000..733fc42
--- /dev/null
+++ b/flink-runtime/src/test/java/org/apache/flink/runtime/metrics/DescriptiveStatisticsHistogramTest.java
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.runtime.metrics;
+
+import org.apache.flink.metrics.AbstractHistogramTest;
+
+import org.junit.Test;
+
+/**
+ * Tests for {@link DescriptiveStatisticsHistogram} and
+ * {@link DescriptiveStatisticsHistogramStatistics}.
+ */
+public class DescriptiveStatisticsHistogramTest extends AbstractHistogramTest {
+
+ /**
+ * Tests the histogram functionality of the DropwizardHistogramWrapper.
+ */
+ @Test
+ public void testDescriptiveHistogram() {
+ int size = 10;
+ testHistogram(size, new DescriptiveStatisticsHistogram(size));
+ }
+
+}