Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.0-alpha
    • Fix Version/s: 2.0.2-alpha
    • Component/s: metrics
    • Labels:
      None
    • Target Version/s:
    • Hadoop Flags:
      Reviewed

      Description

      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.

      1. hadoop-8541-1.patch
        25 kB
        Andrew Wang
      2. hadoop-8541-2.patch
        26 kB
        Andrew Wang
      3. hadoop-8541-3.patch
        26 kB
        Andrew Wang
      4. hadoop-8541-4.patch
        30 kB
        Andrew Wang
      5. hadoop-8541-5.patch
        30 kB
        Andrew Wang
      6. hadoop-8541-6.patch
        30 kB
        Andrew Wang

        Issue Links

          Activity

          Hide
          Aaron T. Myers added a comment -

          +1, this would be a great addition to our existing metrics capabilities.

          Show
          Aaron T. Myers added a comment - +1, this would be a great addition to our existing metrics capabilities.
          Hide
          Andrew Wang added a comment -

          First attempt at a patch. I added a new MutableQuantiles class that tracks hardcoded quantiles, and rolls over at the specified interval.

          The test cases are pretty thorough, but I'm going to work on hooking this into either HDFS or HBase to make sure the API feels right.

          Show
          Andrew Wang added a comment - First attempt at a patch. I added a new MutableQuantiles class that tracks hardcoded quantiles, and rolls over at the specified interval. The test cases are pretty thorough, but I'm going to work on hooking this into either HDFS or HBase to make sure the API feels right.
          Hide
          Aaron T. Myers added a comment -

          Marking PA for Andrew so that Jenkins runs.

          Show
          Aaron T. Myers added a comment - Marking PA for Andrew so that Jenkins runs.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12534486/hadoop-8541-1.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 javadoc. The javadoc tool appears to have generated 1 warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          -1 findbugs. The patch appears to introduce 2 new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common:

          org.apache.hadoop.ha.TestZKFailoverController
          org.apache.hadoop.io.file.tfile.TestTFileByteArrays
          org.apache.hadoop.io.file.tfile.TestTFileJClassComparatorByteArrays

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1168//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/1168//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-common.html
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1168//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12534486/hadoop-8541-1.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified test files. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 javadoc. The javadoc tool appears to have generated 1 warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. -1 findbugs. The patch appears to introduce 2 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common: org.apache.hadoop.ha.TestZKFailoverController org.apache.hadoop.io.file.tfile.TestTFileByteArrays org.apache.hadoop.io.file.tfile.TestTFileJClassComparatorByteArrays +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1168//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/1168//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-common.html Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1168//console This message is automatically generated.
          Hide
          Andrew Wang added a comment -

          Fix the findbugs warnings. I added a filter rule for the synchronization warning; I manually verified it, and the newer version of findbugs correctly recognizes it as safe.

          Show
          Andrew Wang added a comment - Fix the findbugs warnings. I added a filter rule for the synchronization warning; I manually verified it, and the newer version of findbugs correctly recognizes it as safe.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12535283/hadoop-8541-2.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 javadoc. The javadoc tool appears to have generated 1 warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common:

          org.apache.hadoop.ha.TestZKFailoverController
          org.apache.hadoop.io.file.tfile.TestTFileByteArrays
          org.apache.hadoop.io.file.tfile.TestTFileJClassComparatorByteArrays

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1174//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1174//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12535283/hadoop-8541-2.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified test files. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 javadoc. The javadoc tool appears to have generated 1 warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common: org.apache.hadoop.ha.TestZKFailoverController org.apache.hadoop.io.file.tfile.TestTFileByteArrays org.apache.hadoop.io.file.tfile.TestTFileJClassComparatorByteArrays +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1174//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1174//console This message is automatically generated.
          Hide
          Andrew Wang added a comment -

          Fixed the javadoc warning, which should finally make the buildbot happy.

          Reviews appreciated!

          Show
          Andrew Wang added a comment - Fixed the javadoc warning, which should finally make the buildbot happy. Reviews appreciated!
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12535408/hadoop-8541-3.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1177//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1177//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12535408/hadoop-8541-3.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified test files. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The javadoc tool did not generate any warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1177//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1177//console This message is automatically generated.
          Hide
          Aaron T. Myers added a comment -

          Hi Andrew, the patch looks pretty good to me. A few comments:

          1. Need license header in all newly-introduced files.
          2. There's no need for this sort of code in tests. Throwing an exception from a test case will cause the test to fail:
            +      } catch(IOException e) {
            +        e.printStackTrace();
            +        fail("Unexpected exception while fetching estimates from estimator.");
            +        return;
            +      }
            
          3. 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".
          4. 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.
          5. 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.
          6. Could go for some class comments in MutableQuantiles and Quantile classes.
          7. You should put a blank line before instance variable comments, e.g.:
            +  private int bufferCount = 0;
            +  /**
            +   * Array of Quantiles that we care about, along with desired error.
            +   */
            +  private final Quantile quantiles[];
            
          8. Recommend renaming SampleQuantiles#sample to SampleQuantiles#samples.
          9. You should probably pick a more descriptive name for the "Item" class, and perhaps add some comments for the class and its instance variables.
          10. We use lower-camel casing, so rename "lower_delta" to "lowerDelta".
          11. 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?
          12. You should put the "else" on the same line as the associated closing curly brace.
          13. 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?
          14. The "removed" local variable in SampleQuantiles#compress appears to never be used.
          Show
          Aaron T. Myers added a comment - 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: + } catch (IOException e) { + e.printStackTrace(); + fail( "Unexpected exception while fetching estimates from estimator." ); + return ; + } 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.: + private int bufferCount = 0; + /** + * Array of Quantiles that we care about, along with desired error. + */ + private final Quantile quantiles[]; 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.
          Hide
          Andrew Wang added a comment -

          Thanks for the very detailed review Aaron. The Eclipse auto-formatter should have gotten the style comments, and I tried to address most of the rest.

          For 11, since memory usage seemed to be a major concern, I wanted to avoid the object overhead from using Longs instead of the primitive type. This is O(KBs) though, so I can change it if you think it's not readable. The usage of the array is pretty simple though, there isn't any weird iteration, and it's only flushed completely.

          For 13, I'd have to think about how to make this work. It's probably doable (basically need to merge adjacent items instead of inserting), but I don't think it'll yield that big of a performance boost. Again, I'll work on this if you think it's worthwhile.

          Show
          Andrew Wang added a comment - Thanks for the very detailed review Aaron. The Eclipse auto-formatter should have gotten the style comments, and I tried to address most of the rest. For 11, since memory usage seemed to be a major concern, I wanted to avoid the object overhead from using Longs instead of the primitive type. This is O(KBs) though, so I can change it if you think it's not readable. The usage of the array is pretty simple though, there isn't any weird iteration, and it's only flushed completely. For 13, I'd have to think about how to make this work. It's probably doable (basically need to merge adjacent items instead of inserting), but I don't think it'll yield that big of a performance boost. Again, I'll work on this if you think it's worthwhile.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12535890/hadoop-8541-4.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          -1 findbugs. The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common:

          org.apache.hadoop.metrics2.util.TestSampleQuantiles

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1183//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/1183//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-common.html
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1183//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12535890/hadoop-8541-4.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified test files. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The javadoc tool did not generate any warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. -1 findbugs. The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common: org.apache.hadoop.metrics2.util.TestSampleQuantiles +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1183//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/1183//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-common.html Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1183//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12535893/hadoop-8541-4.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          -1 findbugs. The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common:

          org.apache.hadoop.metrics2.util.TestSampleQuantiles

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1184//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/1184//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-common.html
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1184//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12535893/hadoop-8541-4.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified test files. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The javadoc tool did not generate any warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. -1 findbugs. The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common: org.apache.hadoop.metrics2.util.TestSampleQuantiles +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1184//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/1184//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-common.html Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1184//console This message is automatically generated.
          Hide
          Andrew Wang added a comment -

          Little overzealous with try/catch removal, testcases fixed.

          The findbugs warning looks unrelated, I didn't touch those files.

          Show
          Andrew Wang added a comment - Little overzealous with try/catch removal, testcases fixed. The findbugs warning looks unrelated, I didn't touch those files.
          Hide
          Aaron T. Myers added a comment -

          The pre-commit build for this failed for what looks like a transient reason on the Jenkins slave. I've just kicked the build again.

          Show
          Aaron T. Myers added a comment - The pre-commit build for this failed for what looks like a transient reason on the Jenkins slave. I've just kicked the build again.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12535904/hadoop-8541-5.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          -1 findbugs. The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1186//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/1186//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-common.html
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1186//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12535904/hadoop-8541-5.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified test files. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The javadoc tool did not generate any warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. -1 findbugs. The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1186//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/1186//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-common.html Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1186//console This message is automatically generated.
          Hide
          Aaron T. Myers added a comment -

          For 11, since memory usage seemed to be a major concern, I wanted to avoid the object overhead from using Longs instead of the primitive type. This is O(KBs) though, so I can change it if you think it's not readable. The usage of the array is pretty simple though, there isn't any weird iteration, and it's only flushed completely.

          Makes sense, but you might want to add a comment to that effect above the instance vars explaining the reasoning.

          For 13, I'd have to think about how to make this work. It's probably doable (basically need to merge adjacent items instead of inserting), but I don't think it'll yield that big of a performance boost. Again, I'll work on this if you think it's worthwhile.

          I don't think it'll be a performance boost, but the code might be a little clearer. I personally found it a little odd when reviewing it that we would add a bunch of items to the list only to potentially merge them moments later. I don't feel strongly about this, though. We can leave it as you have it if you think the current version is clear enough.

          Some other minor stuff:

          1. You should use GenericTestUtils#assertExceptionContains to ensure that the IOE you actually expect gets thrown, instead of some other unrelated IOE:
            +    try {
            +      estimator.snapshot();
            +      fail("Expected IOException from empty window");
            +    } catch (IOException e) {
            +    }
            
          2. I think you should add a comment explaining the need for this sleep in the tests and why it's calculated the way it is. You should also perhaps use System#currentTimeMillis since I don't think you need nano precision:
            +      long sleep = (start + (5000 * i) + 100) - (System.nanoTime() / 1000000);
            +
            +      Thread.sleep(sleep);
            

          Other than the above the patch looks great. The findbugs warning is HADOOP-8585. +1 once the above issues are addressed, either by fixing them or saying that they're not worth fixing.

          Show
          Aaron T. Myers added a comment - For 11, since memory usage seemed to be a major concern, I wanted to avoid the object overhead from using Longs instead of the primitive type. This is O(KBs) though, so I can change it if you think it's not readable. The usage of the array is pretty simple though, there isn't any weird iteration, and it's only flushed completely. Makes sense, but you might want to add a comment to that effect above the instance vars explaining the reasoning. For 13, I'd have to think about how to make this work. It's probably doable (basically need to merge adjacent items instead of inserting), but I don't think it'll yield that big of a performance boost. Again, I'll work on this if you think it's worthwhile. I don't think it'll be a performance boost, but the code might be a little clearer. I personally found it a little odd when reviewing it that we would add a bunch of items to the list only to potentially merge them moments later. I don't feel strongly about this, though. We can leave it as you have it if you think the current version is clear enough. Some other minor stuff: You should use GenericTestUtils#assertExceptionContains to ensure that the IOE you actually expect gets thrown, instead of some other unrelated IOE: + try { + estimator.snapshot(); + fail( "Expected IOException from empty window" ); + } catch (IOException e) { + } I think you should add a comment explaining the need for this sleep in the tests and why it's calculated the way it is. You should also perhaps use System#currentTimeMillis since I don't think you need nano precision: + long sleep = (start + (5000 * i) + 100) - ( System .nanoTime() / 1000000); + + Thread .sleep(sleep); Other than the above the patch looks great. The findbugs warning is HADOOP-8585 . +1 once the above issues are addressed, either by fixing them or saying that they're not worth fixing.
          Hide
          Andrew Wang added a comment -

          Makes sense, but you might want to add a comment to that effect above the instance vars explaining the reasoning.

          Fixed.

          We can leave it as you have it if you think the current version is clear enough.

          The implementation matches the pseudocode and descriptions in the original CKMS paper, so I'd like to leave the functions separate.

          You should use GenericTestUtils#assertExceptionContains to ensure that the IOE you actually expect gets thrown, instead of some other unrelated IOE

          Fixed.

          I think you should add a comment explaining the need for this sleep in the tests and why it's calculated the way it is. You should also perhaps use System#currentTimeMillis since I don't think you need nano precision

          Comment added. I also bumped the slop from 100ms to 1000ms to hopefully avoid any timing flakiness. I used nanoTime() over currentTimeMillis() since it's a monotonic clock; we wouldn't want our test failing due to leap seconds!

          Show
          Andrew Wang added a comment - Makes sense, but you might want to add a comment to that effect above the instance vars explaining the reasoning. Fixed. We can leave it as you have it if you think the current version is clear enough. The implementation matches the pseudocode and descriptions in the original CKMS paper, so I'd like to leave the functions separate. You should use GenericTestUtils#assertExceptionContains to ensure that the IOE you actually expect gets thrown, instead of some other unrelated IOE Fixed. I think you should add a comment explaining the need for this sleep in the tests and why it's calculated the way it is. You should also perhaps use System#currentTimeMillis since I don't think you need nano precision Comment added. I also bumped the slop from 100ms to 1000ms to hopefully avoid any timing flakiness. I used nanoTime() over currentTimeMillis() since it's a monotonic clock; we wouldn't want our test failing due to leap seconds!
          Hide
          Aaron T. Myers added a comment -

          The implementation matches the pseudocode and descriptions in the original CKMS paper, so I'd like to leave the functions separate.

          Makes sense. Sounds good.

          we wouldn't want our test failing due to leap seconds!

          We certainly wouldn't.

          The latest patch looks good to me. +1 pending Jenkins.

          Show
          Aaron T. Myers added a comment - The implementation matches the pseudocode and descriptions in the original CKMS paper, so I'd like to leave the functions separate. Makes sense. Sounds good. we wouldn't want our test failing due to leap seconds! We certainly wouldn't. The latest patch looks good to me. +1 pending Jenkins.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12536125/hadoop-8541-6.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          -1 findbugs. The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common:

          org.apache.hadoop.ha.TestZKFailoverController
          org.apache.hadoop.io.file.tfile.TestTFileByteArrays
          org.apache.hadoop.io.file.tfile.TestTFileJClassComparatorByteArrays

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1193//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/1193//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-common.html
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1193//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12536125/hadoop-8541-6.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified test files. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The javadoc tool did not generate any warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. -1 findbugs. The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common: org.apache.hadoop.ha.TestZKFailoverController org.apache.hadoop.io.file.tfile.TestTFileByteArrays org.apache.hadoop.io.file.tfile.TestTFileJClassComparatorByteArrays +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1193//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/1193//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-common.html Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1193//console This message is automatically generated.
          Hide
          Aaron T. Myers added a comment -

          The test failures look like HADOOP-8537 and HDFS-3635. The findbugs warning is due to HADOOP-8585.

          I'm going to commit this momentarily.

          Show
          Aaron T. Myers added a comment - The test failures look like HADOOP-8537 and HDFS-3635 . The findbugs warning is due to HADOOP-8585 . I'm going to commit this momentarily.
          Hide
          Aaron T. Myers added a comment -

          I've just committed this to trunk and branch-2. Thanks a lot for the awesome contribution, Andrew.

          Show
          Aaron T. Myers added a comment - I've just committed this to trunk and branch-2. Thanks a lot for the awesome contribution, Andrew.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #2455 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2455/)
          HADOOP-8541. Better high-percentile latency metrics. Contributed by Andrew Wang. (Revision 1360501)

          Result = SUCCESS
          atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1360501
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/dev-support/findbugsExcludeFile.xml
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/lib/MetricsRegistry.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/lib/MutableQuantiles.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/util/Quantile.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/util/SampleQuantiles.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/lib/TestMutableMetrics.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/util/TestSampleQuantiles.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #2455 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2455/ ) HADOOP-8541 . Better high-percentile latency metrics. Contributed by Andrew Wang. (Revision 1360501) Result = SUCCESS atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1360501 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/dev-support/findbugsExcludeFile.xml /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/lib/MetricsRegistry.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/lib/MutableQuantiles.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/util/Quantile.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/util/SampleQuantiles.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/lib/TestMutableMetrics.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/util/TestSampleQuantiles.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #2521 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2521/)
          HADOOP-8541. Better high-percentile latency metrics. Contributed by Andrew Wang. (Revision 1360501)

          Result = SUCCESS
          atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1360501
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/dev-support/findbugsExcludeFile.xml
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/lib/MetricsRegistry.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/lib/MutableQuantiles.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/util/Quantile.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/util/SampleQuantiles.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/lib/TestMutableMetrics.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/util/TestSampleQuantiles.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #2521 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2521/ ) HADOOP-8541 . Better high-percentile latency metrics. Contributed by Andrew Wang. (Revision 1360501) Result = SUCCESS atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1360501 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/dev-support/findbugsExcludeFile.xml /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/lib/MetricsRegistry.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/lib/MutableQuantiles.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/util/Quantile.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/util/SampleQuantiles.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/lib/TestMutableMetrics.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/util/TestSampleQuantiles.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk-Commit #2474 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2474/)
          HADOOP-8541. Better high-percentile latency metrics. Contributed by Andrew Wang. (Revision 1360501)

          Result = FAILURE
          atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1360501
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/dev-support/findbugsExcludeFile.xml
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/lib/MetricsRegistry.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/lib/MutableQuantiles.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/util/Quantile.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/util/SampleQuantiles.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/lib/TestMutableMetrics.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/util/TestSampleQuantiles.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #2474 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2474/ ) HADOOP-8541 . Better high-percentile latency metrics. Contributed by Andrew Wang. (Revision 1360501) Result = FAILURE atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1360501 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/dev-support/findbugsExcludeFile.xml /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/lib/MetricsRegistry.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/lib/MutableQuantiles.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/util/Quantile.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/util/SampleQuantiles.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/lib/TestMutableMetrics.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/util/TestSampleQuantiles.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #1101 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1101/)
          HADOOP-8541. Better high-percentile latency metrics. Contributed by Andrew Wang. (Revision 1360501)

          Result = FAILURE
          atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1360501
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/dev-support/findbugsExcludeFile.xml
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/lib/MetricsRegistry.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/lib/MutableQuantiles.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/util/Quantile.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/util/SampleQuantiles.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/lib/TestMutableMetrics.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/util/TestSampleQuantiles.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #1101 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1101/ ) HADOOP-8541 . Better high-percentile latency metrics. Contributed by Andrew Wang. (Revision 1360501) Result = FAILURE atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1360501 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/dev-support/findbugsExcludeFile.xml /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/lib/MetricsRegistry.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/lib/MutableQuantiles.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/util/Quantile.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/util/SampleQuantiles.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/lib/TestMutableMetrics.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/util/TestSampleQuantiles.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #1134 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1134/)
          HADOOP-8541. Better high-percentile latency metrics. Contributed by Andrew Wang. (Revision 1360501)

          Result = SUCCESS
          atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1360501
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/dev-support/findbugsExcludeFile.xml
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/lib/MetricsRegistry.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/lib/MutableQuantiles.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/util/Quantile.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/util/SampleQuantiles.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/lib/TestMutableMetrics.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/util/TestSampleQuantiles.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1134 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1134/ ) HADOOP-8541 . Better high-percentile latency metrics. Contributed by Andrew Wang. (Revision 1360501) Result = SUCCESS atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1360501 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/dev-support/findbugsExcludeFile.xml /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/lib/MetricsRegistry.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/lib/MutableQuantiles.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/util/Quantile.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/util/SampleQuantiles.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/lib/TestMutableMetrics.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/util/TestSampleQuantiles.java

            People

            • Assignee:
              Andrew Wang
              Reporter:
              Andrew Wang
            • Votes:
              0 Vote for this issue
              Watchers:
              20 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development