[GitHub] [hbase] d-c-manning commented on a change in pull request #1962: HBASE-24615 MutableRangeHistogram#updateSnapshotRangeMetrics doesn't calculate the distribution for last bucket.
d-c-manning commented on a change in pull request #1962:
URL: https://github.com/apache/hbase/pull/1962#discussion_r451224609
##
File path:
hbase-hadoop-compat/src/test/java/org/apache/hadoop/metrics2/impl/TestMutableRangeHistogram.java
##
@@ -0,0 +1,66 @@
+package org.apache.hadoop.metrics2.impl;
+
+import static org.junit.Assert.assertEquals;
+
+import org.apache.hadoop.hbase.HBaseClassTestRule;
+import org.apache.hadoop.hbase.testclassification.MetricsTests;
+import org.apache.hadoop.hbase.testclassification.SmallTests;
+import org.apache.hadoop.metrics2.AbstractMetric;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.lib.MutableRangeHistogram;
+import org.apache.hadoop.metrics2.lib.MutableSizeHistogram;
+import org.apache.hadoop.metrics2.lib.MutableTimeHistogram;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import java.util.Collection;
+
+@Category({ MetricsTests.class, SmallTests.class})
+public class TestMutableRangeHistogram {
+
+@ClassRule
+public static final HBaseClassTestRule CLASS_RULE =
+HBaseClassTestRule.forClass(TestMutableRangeHistogram.class);
+
+private static final String RECORD_NAME = "test";
+private static final String SIZE_METRIC_NAME = "TestSize";
+private static final String TIME_METRIC_NAME = "TestTime";
+
+@Test
+public void testMutableSizeHistogram() {
+testMutableRangeHistogram(SIZE_METRIC_NAME, new
MutableSizeHistogram(SIZE_METRIC_NAME, ""));
+}
+
+@Test
+public void testMutableTimeHistogram() {
+testMutableRangeHistogram(TIME_METRIC_NAME, new
MutableTimeHistogram(TIME_METRIC_NAME, ""));
+}
+
+private static void testMutableRangeHistogram(String metricName,
MutableRangeHistogram histogram) {
+// fill up values
+long[] ranges = histogram.getRanges();
+for (long val : ranges) {
+histogram.add(val - 1);
+}
+histogram.add(ranges[ranges.length - 1] + 1);
+// put into metrics collector
+MetricsCollectorImpl collector = new MetricsCollectorImpl();
+MetricsRecordBuilder builder = collector.addRecord(RECORD_NAME);
+histogram.snapshot(builder, true);
Review comment:
after you call `snapshot` once, the internal bins of `FastLongHistogram`
will reallocate based on the min and max values seen so far. So after this, you
can repeat the calls to `histogram.add` one more time, and then call `snapshot`
a second time. Then the bins will be more accurate (but poor precision for
lower values, due to linear distribution.)
##
File path:
hbase-hadoop-compat/src/test/java/org/apache/hadoop/metrics2/impl/TestMutableRangeHistogram.java
##
@@ -0,0 +1,66 @@
+package org.apache.hadoop.metrics2.impl;
+
+import static org.junit.Assert.assertEquals;
+
+import org.apache.hadoop.hbase.HBaseClassTestRule;
+import org.apache.hadoop.hbase.testclassification.MetricsTests;
+import org.apache.hadoop.hbase.testclassification.SmallTests;
+import org.apache.hadoop.metrics2.AbstractMetric;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.lib.MutableRangeHistogram;
+import org.apache.hadoop.metrics2.lib.MutableSizeHistogram;
+import org.apache.hadoop.metrics2.lib.MutableTimeHistogram;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import java.util.Collection;
+
+@Category({ MetricsTests.class, SmallTests.class})
+public class TestMutableRangeHistogram {
+
+@ClassRule
+public static final HBaseClassTestRule CLASS_RULE =
+HBaseClassTestRule.forClass(TestMutableRangeHistogram.class);
+
+private static final String RECORD_NAME = "test";
+private static final String SIZE_METRIC_NAME = "TestSize";
+private static final String TIME_METRIC_NAME = "TestTime";
+
+@Test
+public void testMutableSizeHistogram() {
+testMutableRangeHistogram(SIZE_METRIC_NAME, new
MutableSizeHistogram(SIZE_METRIC_NAME, ""));
+}
+
+@Test
+public void testMutableTimeHistogram() {
+testMutableRangeHistogram(TIME_METRIC_NAME, new
MutableTimeHistogram(TIME_METRIC_NAME, ""));
+}
+
+private static void testMutableRangeHistogram(String metricName,
MutableRangeHistogram histogram) {
+// fill up values
+long[] ranges = histogram.getRanges();
+for (long val : ranges) {
+histogram.add(val - 1);
+}
+histogram.add(ranges[ranges.length - 1] + 1);
Review comment:
an attempt to choose a value well above the max to ensure it ends up in
the highest bin.
```suggestion
histogram.add(ranges[ranges.length - 1] + ranges[ranges.length - 2]
/ 2);
```
##
File path:
hbase-hadoop-compat/src/test/java/org/apache/hadoop/metrics2/impl/TestMutableRangeHistogram.java
##
@@ -0,0 +1,66 @@
+package o
[GitHub] [hbase] d-c-manning commented on a change in pull request #1962: HBASE-24615 MutableRangeHistogram#updateSnapshotRangeMetrics doesn't calculate the distribution for last bucket.
d-c-manning commented on a change in pull request #1962:
URL: https://github.com/apache/hbase/pull/1962#discussion_r452331427
##
File path:
hbase-hadoop-compat/src/test/java/org/apache/hadoop/metrics2/impl/TestMutableRangeHistogram.java
##
@@ -0,0 +1,90 @@
+/**
+ * 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.hadoop.metrics2.impl;
Review comment:
To match `MutableRangeHistogram` should this be
```suggestion
package org.apache.hadoop.metrics2.lib;
```
##
File path:
hbase-hadoop-compat/src/test/java/org/apache/hadoop/metrics2/impl/TestMutableRangeHistogram.java
##
@@ -0,0 +1,90 @@
+/**
+ * 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.hadoop.metrics2.impl;
+
+import static org.junit.Assert.assertEquals;
+
+import org.apache.hadoop.hbase.HBaseClassTestRule;
+import org.apache.hadoop.hbase.testclassification.MetricsTests;
+import org.apache.hadoop.hbase.testclassification.SmallTests;
+import org.apache.hadoop.metrics2.AbstractMetric;
+import org.apache.hadoop.metrics2.lib.MutableSizeHistogram;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+
+@Category({ MetricsTests.class, SmallTests.class })
+public class TestMutableRangeHistogram {
+
+ @ClassRule
+ public static final HBaseClassTestRule CLASS_RULE =
+HBaseClassTestRule.forClass(TestMutableRangeHistogram.class);
+
+ private static final String RECORD_NAME = "test";
+ private static final String SIZE_HISTOGRAM_NAME = "TestSize";
+
+ /**
+ * calculate the distribution for last bucket, see HBASE-24615 for detail.
+ */
+ @Test
+ public void testLastBucketWithSizeHistogram() {
+// create and init histogram minValue and maxValue
+MetricsCollectorImpl collector = new MetricsCollectorImpl();
+MutableSizeHistogram histogram = new
MutableSizeHistogram(SIZE_HISTOGRAM_NAME, "");
+long[] ranges = histogram.getRanges();
+int len = ranges.length;
+histogram.add(0L);
+histogram.add(ranges[len - 1]);
+histogram.snapshot(collector.addRecord(RECORD_NAME), true);
+collector.clear();
+
+// fill up values and snapshot
+histogram.add(ranges[len - 2] * 2);
+histogram.add(ranges[len - 1] * 2);
+histogram.snapshot(collector.addRecord(RECORD_NAME), true);
+Collection records = collector.getRecords();
+assertEquals(records.size(), 1);
+MetricsRecordImpl record = records.iterator().next();
+assertEquals(record.name(), RECORD_NAME);
Review comment:
```suggestion
assertEquals(RECORD_NAME, record.name());
```
##
File path:
hbase-hadoop-compat/src/test/java/org/apache/hadoop/metrics2/impl/TestMutableRangeHistogram.java
##
@@ -0,0 +1,90 @@
+/**
+ * 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 Licens
