Details

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

Activity

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

Ian Varley
added a comment - 04/Apr/12 20:55 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.)

-----------------------------------------------------------
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.

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.

-----------------------------------------------------------
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.

jiraposter@reviews.apache.org
added a comment - 05/Apr/12 19:16
-----------------------------------------------------------
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

jiraposter@reviews.apache.org
added a comment - 05/Apr/12 19:28
-----------------------------------------------------------
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

Ian Varley
added a comment - 05/Apr/12 19:51 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).

-----------------------------------------------------------
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.

Hadoop QA
added a comment - 05/Apr/12 20:00 -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.

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.

jiraposter@reviews.apache.org
added a comment - 06/Apr/12 22:28
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

Hadoop QA
added a comment - 12/Apr/12 05:55 -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.