[
https://issues.apache.org/jira/browse/HDFS-16982?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17712519#comment-17712519
]
ASF GitHub Bot commented on HDFS-16982:
---------------------------------------
goiri commented on code in PR #5556:
URL: https://github.com/apache/hadoop/pull/5556#discussion_r1167234948
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/lib/MutableQuantiles.java:
##########
@@ -130,11 +129,11 @@ public MutableQuantiles() {}
public synchronized void snapshot(MetricsRecordBuilder builder, boolean all)
{
if (all || changed()) {
builder.addGauge(numInfo, previousCount);
- for (int i = 0; i < quantiles.length; i++) {
+ for (int i = 0; i < getQuantiles().length; i++) {
Review Comment:
You probably need to extract `getQuantiles()`, otherwise you may end up with
two versions.
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/lib/TestMutableMetrics.java:
##########
@@ -431,6 +431,46 @@ public void testMutableQuantilesError() throws Exception {
}
}
+ /**
+ * Ensure that quantile estimates from {@link MutableInverseQuantiles} are
within
+ * specified error bounds.
+ */
+ @Test(timeout = 30000)
+ public void testMutableInverseQuantilesError() throws Exception {
+ MetricsRecordBuilder mb = mockMetricsRecordBuilder();
+ MetricsRegistry registry = new MetricsRegistry("test");
+ // Use a 5s rollover period
+ MutableQuantiles inverseQuantiles = registry.newInverseQuantiles("foo",
"stat", "Ops",
+ "Latency", 5);
+ // Push some values in and wait for it to publish
+ long start = System.nanoTime() / 1000000;
Review Comment:
Use units in the vars.
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/lib/TestMutableMetrics.java:
##########
@@ -431,6 +431,46 @@ public void testMutableQuantilesError() throws Exception {
}
}
+ /**
+ * Ensure that quantile estimates from {@link MutableInverseQuantiles} are
within
+ * specified error bounds.
+ */
+ @Test(timeout = 30000)
+ public void testMutableInverseQuantilesError() throws Exception {
+ MetricsRecordBuilder mb = mockMetricsRecordBuilder();
+ MetricsRegistry registry = new MetricsRegistry("test");
+ // Use a 5s rollover period
+ MutableQuantiles inverseQuantiles = registry.newInverseQuantiles("foo",
"stat", "Ops",
+ "Latency", 5);
+ // Push some values in and wait for it to publish
+ long start = System.nanoTime() / 1000000;
+ for (long i = 1; i <= 1000; i++) {
Review Comment:
Declare constants.
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/lib/TestMutableMetrics.java:
##########
@@ -431,6 +431,46 @@ public void testMutableQuantilesError() throws Exception {
}
}
+ /**
+ * Ensure that quantile estimates from {@link MutableInverseQuantiles} are
within
+ * specified error bounds.
+ */
+ @Test(timeout = 30000)
+ public void testMutableInverseQuantilesError() throws Exception {
+ MetricsRecordBuilder mb = mockMetricsRecordBuilder();
+ MetricsRegistry registry = new MetricsRegistry("test");
+ // Use a 5s rollover period
+ MutableQuantiles inverseQuantiles = registry.newInverseQuantiles("foo",
"stat", "Ops",
+ "Latency", 5);
+ // Push some values in and wait for it to publish
+ long start = System.nanoTime() / 1000000;
+ for (long i = 1; i <= 1000; i++) {
+ inverseQuantiles.add(i);
+ inverseQuantiles.add(1001 - i);
+ }
+ long end = System.nanoTime() / 1000000;
+
+ Thread.sleep(6000 - (end - start));
Review Comment:
Constant
> Use the right Quantiles Array for Inverse Quantiles snapshot
> -------------------------------------------------------------
>
> Key: HDFS-16982
> URL: https://issues.apache.org/jira/browse/HDFS-16982
> Project: Hadoop HDFS
> Issue Type: Bug
> Components: datanode, metrics
> Reporter: Ravindra Dingankar
> Priority: Minor
> Labels: pull-request-available
> Fix For: 3.3.0, 3.4.0
>
>
> HDFS-16949 introduced InverseQuantiles. However during snapshot for Inverse
> Quantiles we were still trying to access values from previous snapshot based
> on the Quantile Array declared in MutableQuantiles. ( Quantile(.50, .050),
> Quantile(.75, .025), Quantile(.90, .010), Quantile(.95, .005), Quantile(.99,
> .001) )
> For InverseQuantiles we wont have these values ( except for Quantile(.50,
> .050) ) thus except for 50 Percentile snapshot wont return any value for the
> remaining quantiles.
> Fix is to use the correct Quantiles Array to retrieve values during snapshot.
> The new UTs verify this behavior.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
