HBase
  1. HBase
  2. HBASE-5717

Scanner metrics are only reported if you get to the end of a scanner

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.94.0, 0.95.0
    • Component/s: Client, metrics
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      When you turn on Scanner Metrics, the metrics are currently only made available if you run over all records available in the scanner. If you stop iterating before the end, the values are never flushed into the metrics object (in the Scan attribute).

      Will supply a patch with fix and test.

      1. ClientScanner_HBASE_5717-v3.patch
        7 kB
        Ian Varley
      2. ClientScanner_HBASE_5717-v2.patch
        6 kB
        Ian Varley
      3. ClientScanner_HBASE_5717.patch
        6 kB
        Ian Varley
      4. 5717-v4.patch
        7 kB
        Ted Yu

        Activity

        Hide
        Hudson added a comment -

        Integrated in HBase-0.94-security #9 (See https://builds.apache.org/job/HBase-0.94-security/9/)
        HBASE-5717 Scanner metrics are only reported if you get to the end of a scanner (Ian Varley) (Revision 1325342)

        Result = SUCCESS
        larsh :
        Files :

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java
        • /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java
        Show
        Hudson added a comment - Integrated in HBase-0.94-security #9 (See https://builds.apache.org/job/HBase-0.94-security/9/ ) HBASE-5717 Scanner metrics are only reported if you get to the end of a scanner (Ian Varley) (Revision 1325342) Result = SUCCESS larsh : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK-security #169 (See https://builds.apache.org/job/HBase-TRUNK-security/169/)
        HBASE-5717 Scanner metrics are only reported if you get to the end of a scanner (Ian Varley) (Revision 1325344)

        Result = FAILURE
        larsh :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK-security #169 (See https://builds.apache.org/job/HBase-TRUNK-security/169/ ) HBASE-5717 Scanner metrics are only reported if you get to the end of a scanner (Ian Varley) (Revision 1325344) Result = FAILURE larsh : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #2745 (See https://builds.apache.org/job/HBase-TRUNK/2745/)
        HBASE-5717 Scanner metrics are only reported if you get to the end of a scanner (Ian Varley) (Revision 1325344)

        Result = SUCCESS
        larsh :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK #2745 (See https://builds.apache.org/job/HBase-TRUNK/2745/ ) HBASE-5717 Scanner metrics are only reported if you get to the end of a scanner (Ian Varley) (Revision 1325344) Result = SUCCESS larsh : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java
        Hide
        Hudson added a comment -

        Integrated in HBase-0.94 #104 (See https://builds.apache.org/job/HBase-0.94/104/)
        HBASE-5717 Scanner metrics are only reported if you get to the end of a scanner (Ian Varley) (Revision 1325342)

        Result = FAILURE
        larsh :
        Files :

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java
        • /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java
        Show
        Hudson added a comment - Integrated in HBase-0.94 #104 (See https://builds.apache.org/job/HBase-0.94/104/ ) HBASE-5717 Scanner metrics are only reported if you get to the end of a scanner (Ian Varley) (Revision 1325342) Result = FAILURE larsh : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java
        Hide
        Lars Hofhansl added a comment -

        Committed to 0.94 and 0.96.
        Thanks for the patch Ian.
        Thanks for the review Ted.

        Show
        Lars Hofhansl added a comment - Committed to 0.94 and 0.96. Thanks for the patch Ian. Thanks for the review Ted.
        Hide
        Hadoop QA added a comment -

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

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

        +1 tests included. The patch appears to include 3 new or modified tests.

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

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

        +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:
        org.apache.hadoop.hbase.master.TestSplitLogManager

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1493//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1493//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1493//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/12522375/5717-v4.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +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: org.apache.hadoop.hbase.master.TestSplitLogManager Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1493//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1493//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1493//console This message is automatically generated.
        Hide
        Ted Yu added a comment -

        Patch v4 changes getScanMetrics() to private.

        Show
        Ted Yu added a comment - Patch v4 changes getScanMetrics() to private.
        Hide
        Ted Yu added a comment -

        @Lars:
        Do you want to integrate the patch ?

        Show
        Ted Yu added a comment - @Lars: Do you want to integrate the patch ?
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-04-04 22:03:32, Ian Varley wrote:

        >

        Good catch. There is another scenario that could be useful when you have long running scan operation. While a scan is running, e.g. before it finishes or closed, if somehow the ScanMetrics can be updated for time to time without perf impact, that can provide more real-time number for mapreduce HBase Ccounters which uses ScanMetrics. But that could be a separate nice-to-have improvement.

        • Ming

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4640/#review6697
        -----------------------------------------------------------

        On 2012-04-05 18:52:50, Ian Varley wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4640/

        -----------------------------------------------------------

        (Updated 2012-04-05 18:52:50)

        Review request for hbase.

        Summary

        -------

        Fix for persistence of scan metrics when the scanner doesn't run to exhaustion.

        This addresses bug HBASE-5717.

        https://issues.apache.org/jira/browse/HBASE-5717

        Diffs

        -----

        /src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1309585

        /src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java 1309585

        Diff: https://reviews.apache.org/r/4640/diff

        Testing

        -------

        Altered the scan metrics unit test to show this problem (now fails without changes to ClientScanner.java).

        Thanks,

        Ian

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-04-04 22:03:32, Ian Varley wrote: > Good catch. There is another scenario that could be useful when you have long running scan operation. While a scan is running, e.g. before it finishes or closed, if somehow the ScanMetrics can be updated for time to time without perf impact, that can provide more real-time number for mapreduce HBase Ccounters which uses ScanMetrics. But that could be a separate nice-to-have improvement. Ming ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4640/#review6697 ----------------------------------------------------------- On 2012-04-05 18:52:50, Ian Varley wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4640/ ----------------------------------------------------------- (Updated 2012-04-05 18:52:50) Review request for hbase. Summary ------- Fix for persistence of scan metrics when the scanner doesn't run to exhaustion. This addresses bug HBASE-5717 . https://issues.apache.org/jira/browse/HBASE-5717 Diffs ----- /src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1309585 /src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java 1309585 Diff: https://reviews.apache.org/r/4640/diff Testing ------- Altered the scan metrics unit test to show this problem (now fails without changes to ClientScanner.java). Thanks, Ian
        Hide
        Hadoop QA added a comment -

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

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

        +1 tests included. The patch appears to include 3 new or modified tests.

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

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

        -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:
        org.apache.hadoop.hbase.master.TestSplitLogManager

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1409//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1409//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1409//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/12521548/ClientScanner_HBASE_5717-v3.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -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: org.apache.hadoop.hbase.master.TestSplitLogManager Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1409//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1409//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1409//console This message is automatically generated.
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-04-05 19:12:39, Lars Hofhansl wrote:

        > /src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java, line 4548

        > <https://reviews.apache.org/r/4640/diff/3/?file=100315#file100315line4548>

        >

        > private?

        Ah yes, sorry, that should be private. Thanks!

        • Ian

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4640/#review6717
        -----------------------------------------------------------

        On 2012-04-05 18:52:50, Ian Varley wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4640/

        -----------------------------------------------------------

        (Updated 2012-04-05 18:52:50)

        Review request for hbase.

        Summary

        -------

        Fix for persistence of scan metrics when the scanner doesn't run to exhaustion.

        This addresses bug HBASE-5717.

        https://issues.apache.org/jira/browse/HBASE-5717

        Diffs

        -----

        /src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1309585

        /src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java 1309585

        Diff: https://reviews.apache.org/r/4640/diff

        Testing

        -------

        Altered the scan metrics unit test to show this problem (now fails without changes to ClientScanner.java).

        Thanks,

        Ian

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-04-05 19:12:39, Lars Hofhansl wrote: > /src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java, line 4548 > < https://reviews.apache.org/r/4640/diff/3/?file=100315#file100315line4548 > > > private? Ah yes, sorry, that should be private. Thanks! Ian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4640/#review6717 ----------------------------------------------------------- On 2012-04-05 18:52:50, Ian Varley wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4640/ ----------------------------------------------------------- (Updated 2012-04-05 18:52:50) Review request for hbase. Summary ------- Fix for persistence of scan metrics when the scanner doesn't run to exhaustion. This addresses bug HBASE-5717 . https://issues.apache.org/jira/browse/HBASE-5717 Diffs ----- /src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1309585 /src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java 1309585 Diff: https://reviews.apache.org/r/4640/diff Testing ------- Altered the scan metrics unit test to show this problem (now fails without changes to ClientScanner.java). Thanks, Ian
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4640/#review6717
        -----------------------------------------------------------

        Ship it!

        Still
        Will fix whitespace and private method on commit.

        /src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java
        <https://reviews.apache.org/r/4640/#comment14540>

        private?

        • Lars

        On 2012-04-05 18:52:50, Ian Varley wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4640/

        -----------------------------------------------------------

        (Updated 2012-04-05 18:52:50)

        Review request for hbase.

        Summary

        -------

        Fix for persistence of scan metrics when the scanner doesn't run to exhaustion.

        This addresses bug HBASE-5717.

        https://issues.apache.org/jira/browse/HBASE-5717

        Diffs

        -----

        /src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1309585

        /src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java 1309585

        Diff: https://reviews.apache.org/r/4640/diff

        Testing

        -------

        Altered the scan metrics unit test to show this problem (now fails without changes to ClientScanner.java).

        Thanks,

        Ian

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4640/#review6717 ----------------------------------------------------------- Ship it! Still Will fix whitespace and private method on commit. /src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java < https://reviews.apache.org/r/4640/#comment14540 > private? Lars On 2012-04-05 18:52:50, Ian Varley wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4640/ ----------------------------------------------------------- (Updated 2012-04-05 18:52:50) Review request for hbase. Summary ------- Fix for persistence of scan metrics when the scanner doesn't run to exhaustion. This addresses bug HBASE-5717 . https://issues.apache.org/jira/browse/HBASE-5717 Diffs ----- /src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1309585 /src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java 1309585 Diff: https://reviews.apache.org/r/4640/diff Testing ------- Altered the scan metrics unit test to show this problem (now fails without changes to ClientScanner.java). Thanks, Ian
        Hide
        Hadoop QA added a comment -

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

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

        +1 tests included. The patch appears to include 3 new or modified tests.

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

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

        -1 findbugs. The patch appears to introduce 3 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:

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1407//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1407//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1407//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/12521539/ClientScanner_HBASE_5717-v2.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 3 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: Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1407//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1407//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1407//console This message is automatically generated.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4640/
        -----------------------------------------------------------

        (Updated 2012-04-05 18:52:50.133346)

        Review request for hbase.

        Changes
        -------

        Added test cases.

        Summary
        -------

        Fix for persistence of scan metrics when the scanner doesn't run to exhaustion.

        This addresses bug HBASE-5717.
        https://issues.apache.org/jira/browse/HBASE-5717

        Diffs (updated)


        /src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1309585
        /src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java 1309585

        Diff: https://reviews.apache.org/r/4640/diff

        Testing
        -------

        Altered the scan metrics unit test to show this problem (now fails without changes to ClientScanner.java).

        Thanks,

        Ian

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4640/ ----------------------------------------------------------- (Updated 2012-04-05 18:52:50.133346) Review request for hbase. Changes ------- Added test cases. Summary ------- Fix for persistence of scan metrics when the scanner doesn't run to exhaustion. This addresses bug HBASE-5717 . https://issues.apache.org/jira/browse/HBASE-5717 Diffs (updated) /src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1309585 /src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java 1309585 Diff: https://reviews.apache.org/r/4640/diff Testing ------- Altered the scan metrics unit test to show this problem (now fails without changes to ClientScanner.java). Thanks, Ian
        Hide
        Ian Varley added a comment -

        Added additional test cases for running off the end of the available results without closing the scanner, and also doing so with closing the scanner (so, all 3 permutations are covered).

        Show
        Ian Varley added a comment - Added additional test cases for running off the end of the available results without closing the scanner, and also doing so with closing the scanner (so, all 3 permutations are covered).
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4640/#review6715
        -----------------------------------------------------------

        Ship it!

        The only comment I have is that there is now no test that verifies that metrics are collected when we scanned all the way to the end, but did not close the scanner.

        • Lars

        On 2012-04-05 18:15:41, Ian Varley wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4640/

        -----------------------------------------------------------

        (Updated 2012-04-05 18:15:41)

        Review request for hbase.

        Summary

        -------

        Fix for persistence of scan metrics when the scanner doesn't run to exhaustion.

        This addresses bug HBASE-5717.

        https://issues.apache.org/jira/browse/HBASE-5717

        Diffs

        -----

        /src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java 1309585

        /src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1309585

        Diff: https://reviews.apache.org/r/4640/diff

        Testing

        -------

        Altered the scan metrics unit test to show this problem (now fails without changes to ClientScanner.java).

        Thanks,

        Ian

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4640/#review6715 ----------------------------------------------------------- Ship it! The only comment I have is that there is now no test that verifies that metrics are collected when we scanned all the way to the end, but did not close the scanner. Lars On 2012-04-05 18:15:41, Ian Varley wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4640/ ----------------------------------------------------------- (Updated 2012-04-05 18:15:41) Review request for hbase. Summary ------- Fix for persistence of scan metrics when the scanner doesn't run to exhaustion. This addresses bug HBASE-5717 . https://issues.apache.org/jira/browse/HBASE-5717 Diffs ----- /src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java 1309585 /src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1309585 Diff: https://reviews.apache.org/r/4640/diff Testing ------- Altered the scan metrics unit test to show this problem (now fails without changes to ClientScanner.java). Thanks, Ian
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4640/
        -----------------------------------------------------------

        (Updated 2012-04-05 18:15:41.419224)

        Review request for hbase.

        Changes
        -------

        Added finally block (with try/catch inside). I'd prefer to also add logging rather than swallowing the exceptions, but that seems like it should be a different Jira (and maybe should cover all cases that swallow exceptions).

        Also: sorry for a) sending a review on the board for something that's probably too small, and b) writing comments on my own review (intended to annotate, didn't realize it would appear as if I were a separate reviewer).

        Summary
        -------

        Fix for persistence of scan metrics when the scanner doesn't run to exhaustion.

        This addresses bug HBASE-5717.
        https://issues.apache.org/jira/browse/HBASE-5717

        Diffs (updated)


        /src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java 1309585
        /src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1309585

        Diff: https://reviews.apache.org/r/4640/diff

        Testing
        -------

        Altered the scan metrics unit test to show this problem (now fails without changes to ClientScanner.java).

        Thanks,

        Ian

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4640/ ----------------------------------------------------------- (Updated 2012-04-05 18:15:41.419224) Review request for hbase. Changes ------- Added finally block (with try/catch inside). I'd prefer to also add logging rather than swallowing the exceptions, but that seems like it should be a different Jira (and maybe should cover all cases that swallow exceptions). Also: sorry for a) sending a review on the board for something that's probably too small, and b) writing comments on my own review (intended to annotate, didn't realize it would appear as if I were a separate reviewer). Summary ------- Fix for persistence of scan metrics when the scanner doesn't run to exhaustion. This addresses bug HBASE-5717 . https://issues.apache.org/jira/browse/HBASE-5717 Diffs (updated) /src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java 1309585 /src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1309585 Diff: https://reviews.apache.org/r/4640/diff Testing ------- Altered the scan metrics unit test to show this problem (now fails without changes to ClientScanner.java). Thanks, Ian
        Hide
        Ted Yu added a comment -

        Patch v2 looks good to me.

        Show
        Ted Yu added a comment - Patch v2 looks good to me.
        Hide
        Ian Varley added a comment -

        Updated to put writeScanMetrics in finally block

        Show
        Ian Varley added a comment - Updated to put writeScanMetrics in finally block
        Hide
        Hadoop QA added a comment -

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

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

        +1 tests included. The patch appears to include 3 new or modified tests.

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

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

        -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:
        org.apache.hadoop.hbase.mapreduce.TestMultithreadedTableMapper
        org.apache.hadoop.hbase.mapreduce.TestImportTsv
        org.apache.hadoop.hbase.mapred.TestTableMapReduce
        org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat
        org.apache.hadoop.hbase.mapreduce.TestTableMapReduce

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1392//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1392//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1392//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/12521397/ClientScanner_HBASE_5717.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -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: org.apache.hadoop.hbase.mapreduce.TestMultithreadedTableMapper org.apache.hadoop.hbase.mapreduce.TestImportTsv org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat org.apache.hadoop.hbase.mapreduce.TestTableMapReduce Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1392//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1392//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1392//console This message is automatically generated.
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-04-04 22:07:52, Ted Yu wrote:

        > /src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java, line 376

        > <https://reviews.apache.org/r/4640/diff/1/?file=100049#file100049line376>

        >

        > Should this call be placed in a finally block ?

        Yes, you're right, that'd be cleaner (I took a shortcut putting it here b/c it also throws IOException, and we'd have to swallow it the same way in another try block inside the finally block, but you're right, that's more correct). Will append the patch with that.

        • Ian

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4640/#review6698
        -----------------------------------------------------------

        On 2012-04-04 22:00:42, Ian Varley wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4640/

        -----------------------------------------------------------

        (Updated 2012-04-04 22:00:42)

        Review request for hbase.

        Summary

        -------

        Fix for persistence of scan metrics when the scanner doesn't run to exhaustion.

        This addresses bug HBASE-5717.

        https://issues.apache.org/jira/browse/HBASE-5717

        Diffs

        -----

        /src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java 1309585

        /src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1309585

        Diff: https://reviews.apache.org/r/4640/diff

        Testing

        -------

        Altered the scan metrics unit test to show this problem (now fails without changes to ClientScanner.java).

        Thanks,

        Ian

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-04-04 22:07:52, Ted Yu wrote: > /src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java, line 376 > < https://reviews.apache.org/r/4640/diff/1/?file=100049#file100049line376 > > > Should this call be placed in a finally block ? Yes, you're right, that'd be cleaner (I took a shortcut putting it here b/c it also throws IOException, and we'd have to swallow it the same way in another try block inside the finally block, but you're right, that's more correct). Will append the patch with that. Ian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4640/#review6698 ----------------------------------------------------------- On 2012-04-04 22:00:42, Ian Varley wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4640/ ----------------------------------------------------------- (Updated 2012-04-04 22:00:42) Review request for hbase. Summary ------- Fix for persistence of scan metrics when the scanner doesn't run to exhaustion. This addresses bug HBASE-5717 . https://issues.apache.org/jira/browse/HBASE-5717 Diffs ----- /src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java 1309585 /src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1309585 Diff: https://reviews.apache.org/r/4640/diff Testing ------- Altered the scan metrics unit test to show this problem (now fails without changes to ClientScanner.java). Thanks, Ian
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4640/#review6698
        -----------------------------------------------------------

        /src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java
        <https://reviews.apache.org/r/4640/#comment14504>

        Should this call be placed in a finally block ?

        • Ted

        On 2012-04-04 22:00:42, Ian Varley wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4640/

        -----------------------------------------------------------

        (Updated 2012-04-04 22:00:42)

        Review request for hbase.

        Summary

        -------

        Fix for persistence of scan metrics when the scanner doesn't run to exhaustion.

        This addresses bug HBASE-5717.

        https://issues.apache.org/jira/browse/HBASE-5717

        Diffs

        -----

        /src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java 1309585

        /src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1309585

        Diff: https://reviews.apache.org/r/4640/diff

        Testing

        -------

        Altered the scan metrics unit test to show this problem (now fails without changes to ClientScanner.java).

        Thanks,

        Ian

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4640/#review6698 ----------------------------------------------------------- /src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java < https://reviews.apache.org/r/4640/#comment14504 > Should this call be placed in a finally block ? Ted On 2012-04-04 22:00:42, Ian Varley wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4640/ ----------------------------------------------------------- (Updated 2012-04-04 22:00:42) Review request for hbase. Summary ------- Fix for persistence of scan metrics when the scanner doesn't run to exhaustion. This addresses bug HBASE-5717 . https://issues.apache.org/jira/browse/HBASE-5717 Diffs ----- /src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java 1309585 /src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1309585 Diff: https://reviews.apache.org/r/4640/diff Testing ------- Altered the scan metrics unit test to show this problem (now fails without changes to ClientScanner.java). Thanks, Ian
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4640/#review6697
        -----------------------------------------------------------

        /src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java
        <https://reviews.apache.org/r/4640/#comment14500>

        No need for this here any more, since it's called in close()

        /src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java
        <https://reviews.apache.org/r/4640/#comment14501>

        Linebreak change (100 characters now)

        /src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java
        <https://reviews.apache.org/r/4640/#comment14502>

        Important change: writing the scan metrics on scanner close

        /src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java
        <https://reviews.apache.org/r/4640/#comment14503>

        This would fail w/o the changes to ClientScanner, because neither next() nor close() would persist the scan metrics out if we don't run off the end of the available records.

        • Ian

        On 2012-04-04 22:00:42, Ian Varley wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4640/

        -----------------------------------------------------------

        (Updated 2012-04-04 22:00:42)

        Review request for hbase.

        Summary

        -------

        Fix for persistence of scan metrics when the scanner doesn't run to exhaustion.

        This addresses bug HBASE-5717.

        https://issues.apache.org/jira/browse/HBASE-5717

        Diffs

        -----

        /src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java 1309585

        /src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1309585

        Diff: https://reviews.apache.org/r/4640/diff

        Testing

        -------

        Altered the scan metrics unit test to show this problem (now fails without changes to ClientScanner.java).

        Thanks,

        Ian

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4640/#review6697 ----------------------------------------------------------- /src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java < https://reviews.apache.org/r/4640/#comment14500 > No need for this here any more, since it's called in close() /src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java < https://reviews.apache.org/r/4640/#comment14501 > Linebreak change (100 characters now) /src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java < https://reviews.apache.org/r/4640/#comment14502 > Important change: writing the scan metrics on scanner close /src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java < https://reviews.apache.org/r/4640/#comment14503 > This would fail w/o the changes to ClientScanner, because neither next() nor close() would persist the scan metrics out if we don't run off the end of the available records. Ian On 2012-04-04 22:00:42, Ian Varley wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4640/ ----------------------------------------------------------- (Updated 2012-04-04 22:00:42) Review request for hbase. Summary ------- Fix for persistence of scan metrics when the scanner doesn't run to exhaustion. This addresses bug HBASE-5717 . https://issues.apache.org/jira/browse/HBASE-5717 Diffs ----- /src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java 1309585 /src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1309585 Diff: https://reviews.apache.org/r/4640/diff Testing ------- Altered the scan metrics unit test to show this problem (now fails without changes to ClientScanner.java). Thanks, Ian
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4640/
        -----------------------------------------------------------

        Review request for hbase.

        Summary
        -------

        Fix for persistence of scan metrics when the scanner doesn't run to exhaustion.

        This addresses bug HBASE-5717.
        https://issues.apache.org/jira/browse/HBASE-5717

        Diffs


        /src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java 1309585
        /src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1309585

        Diff: https://reviews.apache.org/r/4640/diff

        Testing
        -------

        Altered the scan metrics unit test to show this problem (now fails without changes to ClientScanner.java).

        Thanks,

        Ian

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4640/ ----------------------------------------------------------- Review request for hbase. Summary ------- Fix for persistence of scan metrics when the scanner doesn't run to exhaustion. This addresses bug HBASE-5717 . https://issues.apache.org/jira/browse/HBASE-5717 Diffs /src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java 1309585 /src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1309585 Diff: https://reviews.apache.org/r/4640/diff Testing ------- Altered the scan metrics unit test to show this problem (now fails without changes to ClientScanner.java). Thanks, Ian
        Hide
        Ian Varley added a comment -

        Patch to fix scanner metrics propagation when not exhausting all records in a scan

        Show
        Ian Varley added a comment - Patch to fix scanner metrics propagation when not exhausting all records in a scan
        Hide
        Ian Varley added a comment -

        The problem is in ClientScanner. writeScanMetrics() is called from two places in next(), and if your last "next" doesn't result in no more results, it's never called. Fix should be to call it then, but also call it explicitly from close(). (That allows removing the first call in next() as well, as it's superfluous then.)

        Show
        Ian Varley added a comment - The problem is in ClientScanner. writeScanMetrics() is called from two places in next(), and if your last "next" doesn't result in no more results, it's never called. Fix should be to call it then, but also call it explicitly from close(). (That allows removing the first call in next() as well, as it's superfluous then.)

          People

          • Assignee:
            Ian Varley
            Reporter:
            Ian Varley
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 4h
              4h
              Remaining:
              Remaining Estimate - 4h
              4h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development