inverse for loop review comment

Author: Ravindra Dingankar

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/lib/MetricsRegistry.java

@@ -215,12 +215,12 @@ public synchronized MutableGaugeFloat newGauge(MetricsInfo info, float iVal) {
    * @throws MetricsException if interval is not a positive integer
    */
   public synchronized MutableQuantiles newQuantiles(String name, String desc,
-      String sampleName, String valueName, int interval) {
-      return newQuantiles(name, desc, sampleName, valueName, interval, false);
+    String sampleName, String valueName, int interval) {
+    return newQuantiles(name, desc, sampleName, valueName, interval, false);
   }
 
   /**
-   * Create a mutable metric that estimates quantiles of a stream of values
+   * Create a mutable metric that estimates quantiles of a stream of values.
    * @param name of the metric
    * @param desc metric description
    * @param sampleName of the metric (e.g., "Ops")
@@ -230,7 +230,8 @@ public synchronized MutableQuantiles newQuantiles(String name, String desc,
    * @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/util/InverseQuantiles.java

@@ -18,29 +18,38 @@
 
 package org.apache.hadoop.metrics2.util;
 
+import java.util.ListIterator;
+
 public class InverseQuantiles extends SampleQuantiles{
 
   public InverseQuantiles(Quantile[] quantiles) {
     super(quantiles);
   }
 
   /**
-   * 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.
+   * 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 that quantile.
    */
-  int getDesiredLocation(final double quantile, final long count) {
-    return (int) ((1 - quantile) * count);
-  }
+  long query(double quantile) {
+    int rankMin = 0;
+    int desired = (int) (quantile * getCount());
 
-  /**
-   * Return the best (minimum) value from given sample
-   * @return minimum value from given sample
-   */
-  long getMaxValue() {
-    return samples.get(0).value;
+    ListIterator<SampleItem> it = getSamples().listIterator(getSampleCount());
+    SampleItem prev;
+    SampleItem cur = it.previous();
+    for (int i = 1; i < getSamples().size(); i++) {
+      prev = cur;
+      cur = it.previous();
+      rankMin += prev.g;
+      if (rankMin + cur.g + cur.delta > desired + (allowableError(i) / 2)) {
+        return prev.value;
+      }
+    }
+
+    // edge case of wanting min value
+    return getSamples().get(0).value;
   }
 }

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/util/SampleQuantiles.java

@@ -57,7 +57,7 @@ public class SampleQuantiles implements QuantileEstimator {
   /**
    * Current list of sampled items, maintained in sorted order with error bounds
    */
-  LinkedList<SampleItem> samples;
+  private LinkedList<SampleItem> samples;
 
   /**
    * Buffers incoming items to be inserted in batch. Items are inserted into 
@@ -87,7 +87,7 @@ public SampleQuantiles(Quantile[] quantiles) {
    * @param rank
    *          the index in the list of samples
    */
-  private double allowableError(int rank) {
+  double allowableError(int rank) {
     int size = samples.size();
     double minError = size + 1;
     for (Quantile q : quantiles) {
@@ -203,11 +203,9 @@ private void compress() {
    * @param quantile Queried quantile, e.g. 0.50 or 0.99.
    * @return Estimated value at that quantile.
    */
-  private long query(double quantile) {
-    Preconditions.checkState(!samples.isEmpty(), "no data in estimator");
-
+  long query(double quantile) {
     int rankMin = 0;
-    int desired = getDesiredLocation(quantile, count);
+    int desired = (int) (quantile * count);
 
     ListIterator<SampleItem> it = samples.listIterator();
     SampleItem prev = null;
@@ -223,28 +221,8 @@ private long query(double quantile) {
       }
     }
 
-    // edge case of wanting the best value
-    return getMaxValue();
-  }
-
-  /**
-   * 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;    
+    // edge case of wanting max value
+    return samples.get(samples.size() - 1).value;
   }
 
   /**
@@ -263,6 +241,7 @@ synchronized public Map<Quantile, Long> snapshot() {
 
     Map<Quantile, Long> values = new TreeMap<Quantile, Long>();
     for (int i = 0; i < quantiles.length; i++) {
+      Preconditions.checkState(!samples.isEmpty(), "no data in estimator");
       values.put(quantiles[i], query(quantiles[i].quantile));
     }
 
@@ -278,6 +257,14 @@ synchronized public long getCount() {
     return count;
   }
 
+  /**
+   * Returns the list of samples
+   *
+   * @return samples list of samples
+   */
+  synchronized public LinkedList<SampleItem> getSamples() {
+    return samples;
+  }
   /**
    * Returns the number of samples kept by the estimator
    * 
@@ -311,7 +298,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

@@ -120,8 +120,8 @@ 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.
+   * 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() {

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/metrics/DataNodeMetrics.java

@@ -260,8 +260,8 @@ public DataNodeMetrics(String name, String sessionId, int[] intervals,
           "ops", "latency", interval);
       readTransferRateQuantiles[i] = registry.newQuantiles(
           "readTransferRate" + interval + "s",
-          "Rate at which bytes are read from datanode calculated in bytes per second with inverse quantiles",
-          "ops", "rate", interval, true);
+          "Rate at which bytes are read from datanode calculated in bytes per second" +
+          " with inverse quantiles", "ops", "rate", interval, true);
     }
   }