From 55c76d8a5dc99116ba3a1cfd1cae635db84bcbec Mon Sep 17 00:00:00 2001 From: Ravindra Dingankar Date: Fri, 17 Mar 2023 10:32:33 -0700 Subject: [PATCH] review comments and fix spaces --- .../hadoop/metrics2/lib/MetricsRegistry.java | 4 +- .../hadoop/metrics2/lib/MutableQuantiles.java | 2 + .../metrics2/util/InverseQuantiles.java | 62 ++++++++++--------- .../hadoop/metrics2/util/SampleQuantiles.java | 32 ++++++++-- .../metrics2/util/TestSampleQuantiles.java | 57 +++++++++-------- 5 files changed, 92 insertions(+), 65 deletions(-) diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/lib/MetricsRegistry.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/lib/MetricsRegistry.java index 2e0c90c1dedc2a..bf79c27c82e5b0 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/lib/MetricsRegistry.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/lib/MetricsRegistry.java @@ -226,11 +226,11 @@ public synchronized MutableQuantiles newQuantiles(String name, String desc, * @param sampleName of the metric (e.g., "Ops") * @param valueName of the metric (e.g., "Time" or "Latency") * @param interval rollover interval of estimator in seconds - * @param inverseQuantiles inverse the quantiles ( e.g. P99 will give the 1st quantile ) + * @param inverseQuantiles inverse the quantiles ( e.g. P99 will give the 1st quantile ) * @return a new quantile estimator object * @throws MetricsException if interval is not a positive integer */ - public synchronized MutableQuantiles newQuantiles(String name, String desc, String sampleName, String valueName, + public synchronized MutableQuantiles newQuantiles(String name, String desc, String sampleName, String valueName, int interval, boolean inverseQuantiles) { checkMetricName(name); if (interval <= 0) { diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/lib/MutableQuantiles.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/lib/MutableQuantiles.java index e8c09aff8b4a49..5e3e043d79fb6e 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/lib/MutableQuantiles.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/lib/MutableQuantiles.java @@ -82,6 +82,8 @@ public class MutableQuantiles extends MutableMetric { * type of the values * @param interval * rollover interval (in seconds) of the estimator + * @param inverseQuantiles + * flag to denote if inverse quantiles are requested */ public MutableQuantiles(String name, String description, String sampleName, String valueName, int interval, boolean inverseQuantiles) { diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/util/InverseQuantiles.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/util/InverseQuantiles.java index 3f5261d45213c2..100fc7614bf7ee 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/util/InverseQuantiles.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/util/InverseQuantiles.java @@ -1,44 +1,46 @@ -package org.apache.hadoop.metrics2.util; +/** + * 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. + */ -import org.apache.hadoop.util.Preconditions; -import java.util.ListIterator; +package org.apache.hadoop.metrics2.util; public class InverseQuantiles extends SampleQuantiles{ public InverseQuantiles(Quantile[] quantiles) { super(quantiles); } - /** - * Get the estimated value at the inverse of the specified quantile. - * Eg: return the value at (1 - 0.99)*count position for quantile 0.99. - * When count is 100, quantile 0.99 is desired to return the value at the 1st position - * - * @param quantile Queried quantile, e.g. 0.50 or 0.99. - * @return Estimated value at the inverse position of that quantile. + * Get the desired location from the sample for inverse of the specified quantile. + * Eg: return (1 - 0.99)*count position for quantile 0.99. + * When count is 100, the desired location for quantile 0.99 is the 1st position + * @param quantile queried quantile, e.g. 0.50 or 0.99. + * @param count sample size count + * @return Desired location inverse position of that quantile. */ - long query(double quantile) { - Preconditions.checkState(!samples.isEmpty(), "no data in estimator"); - - int rankMin = 0; - int desired = (int) ((1 - quantile) * count); - - ListIterator it = samples.listIterator(); - SampleItem prev; - SampleItem cur = it.next(); - for (int i = 1; i < samples.size(); i++) { - prev = cur; - cur = it.next(); - - rankMin += prev.g; - - if (rankMin + cur.g + cur.delta > desired + (allowableError(i) / 2)) { - return prev.value; - } - } + int getDesiredLocation(final double quantile, final long count) { + return (int) ((1 - quantile) * count); + } - // edge case of wanting max value + /** + * Return the best (minimum) value from given sample + * @return minimum value from given sample + */ + long getMaxValue() { return samples.get(0).value; } } diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/util/SampleQuantiles.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/util/SampleQuantiles.java index 122dd9ef04fca1..64c43ad44fec45 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/util/SampleQuantiles.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/util/SampleQuantiles.java @@ -52,7 +52,7 @@ public class SampleQuantiles implements QuantileEstimator { /** * Total number of items in stream */ - long count = 0; + private long count = 0; /** * Current list of sampled items, maintained in sorted order with error bounds @@ -87,7 +87,7 @@ public SampleQuantiles(Quantile[] quantiles) { * @param rank * the index in the list of samples */ - double allowableError(int rank) { + private double allowableError(int rank) { int size = samples.size(); double minError = size + 1; for (Quantile q : quantiles) { @@ -203,11 +203,11 @@ private void compress() { * @param quantile Queried quantile, e.g. 0.50 or 0.99. * @return Estimated value at that quantile. */ - long query(double quantile) { + private long query(double quantile) { Preconditions.checkState(!samples.isEmpty(), "no data in estimator"); int rankMin = 0; - int desired = (int) (quantile * count); + int desired = getDesiredLocation(quantile, count); ListIterator it = samples.listIterator(); SampleItem prev = null; @@ -223,10 +223,30 @@ long query(double quantile) { } } - // edge case of wanting max value + // edge case of wanting the best value return samples.get(samples.size() - 1).value; } + /** + * Get the desired location from the sample for inverse of the specified quantile. + * Eg: return (1 - 0.99)*count position for quantile 0.99. + * When count is 100, the desired location for quantile 0.99 is the 1st position + * @param quantile queried quantile, e.g. 0.50 or 0.99. + * @param count sample size count + * @return Desired location inverse position of that quantile. + */ + int getDesiredLocation(final double quantile, final long count) { + return (int) (quantile * count); + } + + /** + * Return the best (maximum) value from given sample + * @return maximum value from given sample + */ + long getMaxValue() { + return samples.get(samples.size() - 1).value; + } + /** * Get a snapshot of the current values of all the tracked quantiles. * @@ -291,7 +311,7 @@ synchronized public String toString() { * Describes a measured value passed to the estimator, tracking additional * metadata required by the CKMS algorithm. */ - static class SampleItem { + static class SampleItem { /** * Value of the sampled item (e.g. a measured latency value) diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/util/TestSampleQuantiles.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/util/TestSampleQuantiles.java index ff6bcf88262896..97714f24f09b0a 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/util/TestSampleQuantiles.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/util/TestSampleQuantiles.java @@ -118,36 +118,39 @@ public void testQuantileError() throws IOException { } } } - + + /** + * Correctness test that checks that absolute error of the estimate for inverse quantiles is within + * specified error bounds for some randomly permuted streams of items. + */ @Test public void testInverseQuantiles() { - SampleQuantiles estimatorWithInverseQuantiles = new InverseQuantiles(quantiles); - final int count = 100000; - Random r = new Random(0xDEADDEAD); - Long[] values = new Long[count]; - for (int i = 0; i < count; i++) { - values[i] = (long) (i + 1); + SampleQuantiles estimatorWithInverseQuantiles = new InverseQuantiles(quantiles); + final int count = 100000; + Random r = new Random(0xDEADDEAD); + Long[] values = new Long[count]; + for (int i = 0; i < count; i++) { + values[i] = (long) (i + 1); + } + // Do 10 shuffle/insert/check cycles + for (int i = 0; i < 10; i++) { + System.out.println("Starting run " + i); + Collections.shuffle(Arrays.asList(values), r); + estimatorWithInverseQuantiles.clear(); + for (int j = 0; j < count; j++) { + estimatorWithInverseQuantiles.insert(values[j]); } - // Do 10 shuffle/insert/check cycles - for (int i = 0; i < 10; i++) { - System.out.println("Starting run " + i); - Collections.shuffle(Arrays.asList(values), r); - estimatorWithInverseQuantiles.clear(); - for (int j = 0; j < count; j++) { - estimatorWithInverseQuantiles.insert(values[j]); - } - Map snapshot; - snapshot = estimatorWithInverseQuantiles.snapshot(); - for (Quantile q : quantiles) { - long actual = (long) ((1 - q.quantile) * count); - long error = (long) ((0.1 - q.error) * count); - long estimate = snapshot.get(q); - System.out - .println(String.format("For quantile %f Expected %d with error %d, estimated %d", - q.quantile, actual, error, estimate)); - assertThat(estimate <= actual + error).isTrue(); - assertThat(estimate >= actual - error).isTrue(); - } + Map snapshot; + snapshot = estimatorWithInverseQuantiles.snapshot(); + for (Quantile q : quantiles) { + long actual = (long) ((1 - q.quantile) * count); + long error = (long) ((0.1 - q.error) * count); + long estimate = snapshot.get(q); + System.out.println(String.format("For quantile %f Expected %d with error %d, estimated %d", + q.quantile, actual, error, estimate)); + assertThat(estimate <= actual + error).isTrue(); + assertThat(estimate >= actual - error).isTrue(); } + } } }