[
https://issues.apache.org/jira/browse/HADOOP-8541?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13410068#comment-13410068
]
Aaron T. Myers commented on HADOOP-8541:
----------------------------------------
Hi Andrew, the patch looks pretty good to me. A few comments:
# Need license header in all newly-introduced files.
# There's no need for this sort of code in tests. Throwing an exception from a
test case will cause the test to fail:
{code}
+ } catch(IOException e) {
+ e.printStackTrace();
+ fail("Unexpected exception while fetching estimates from estimator.");
+ return;
+ }
{code}
# Our coding conventions say to put a single space between conditionals and
opening parens, e.g. "if (...)", and spaces around operators, e.g. "i = 0".
# Why are the instance variables in MutableQuantiles not private? If it's for
testing, then they should be marked with @VisibleForTesting, or made private
and exposed via getters/setters that are marked @VisibleForTesting.
# You should make the inner class RolloverSample static if possible. Given that
that class has a reference to the appropriate MutableQuantiles object, that
shouldn't be hard.
# Could go for some class comments in MutableQuantiles and Quantile classes.
# You should put a blank line before instance variable comments, e.g.:
{code}
+ private int bufferCount = 0;
+ /**
+ * Array of Quantiles that we care about, along with desired error.
+ */
+ private final Quantile quantiles[];
{code}
# Recommend renaming SampleQuantiles#sample to SampleQuantiles#samples.
# You should probably pick a more descriptive name for the "Item" class, and
perhaps add some comments for the class and its instance variables.
# We use lower-camel casing, so rename "lower_delta" to "lowerDelta".
# Rather than keeping the buffer of samples in an array and manually keeping
track of the size of the array, how about just using an ArrayList or the like
and calling List#sort?
# You should put the "else" on the same line as the associated closing curly
brace.
# Rather than doing a full merge of the contents of the buffer into the samples
list, and then performing a compression operation to remove some samples, is it
not possible to somehow only perform an insert into the samples set if it's
within the allowable error bounds?
# The "removed" local variable in SampleQuantiles#compress appears to never be
used.
> Better high-percentile latency metrics
> --------------------------------------
>
> Key: HADOOP-8541
> URL: https://issues.apache.org/jira/browse/HADOOP-8541
> Project: Hadoop Common
> Issue Type: Improvement
> Components: metrics
> Affects Versions: 2.0.0-alpha
> Reporter: Andrew Wang
> Assignee: Andrew Wang
> Attachments: hadoop-8541-1.patch, hadoop-8541-2.patch,
> hadoop-8541-3.patch
>
>
> Based on discussion in HBASE-6261 and with some HDFS devs, I'd like to make
> better high-percentile latency metrics a part of hadoop-common.
> I've already got a working implementation of [1], an efficient algorithm for
> estimating quantiles on a stream of values. It allows you to specify
> arbitrary quantiles to track (e.g. 50th, 75th, 90th, 95th, 99th), along with
> tight error bounds. This estimator can be snapshotted and reset periodically
> to get a feel for how these percentiles are changing over time.
> I propose creating a new MutableQuantiles class that does this. [1] isn't
> completely without overhead (~1MB memory for reasonably sized windows), which
> is why I hesitate to add it to the existing MutableStat class.
> [1] Cormode, Korn, Muthukrishnan, and Srivastava. "Effective Computation of
> Biased Quantiles over Data Streams" in ICDE 2005.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
