review comments and fix spaces

Author: Ravindra Dingankar

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) {

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) {

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<SampleItem> 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;
   }
 }

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<SampleItem> 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)

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<Quantile, Long> 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<Quantile, Long> 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();
       }
+    }
   }
 }