You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hbase.apache.org by te...@apache.org on 2015/09/29 23:12:39 UTC
hbase git commit: HBASE-14497 Reverse Scan threw StackOverflow caused
by readPt checking (Yerui Sun)
Repository: hbase
Updated Branches:
refs/heads/master dbbef0613 -> 653458365
HBASE-14497 Reverse Scan threw StackOverflow caused by readPt checking (Yerui Sun)
Project: http://git-wip-us.apache.org/repos/asf/hbase/repo
Commit: http://git-wip-us.apache.org/repos/asf/hbase/commit/65345836
Tree: http://git-wip-us.apache.org/repos/asf/hbase/tree/65345836
Diff: http://git-wip-us.apache.org/repos/asf/hbase/diff/65345836
Branch: refs/heads/master
Commit: 653458365281d96c288804d094c7d5f8d826a7d7
Parents: dbbef06
Author: tedyu <yu...@gmail.com>
Authored: Tue Sep 29 14:12:24 2015 -0700
Committer: tedyu <yu...@gmail.com>
Committed: Tue Sep 29 14:12:24 2015 -0700
----------------------------------------------------------------------
.../hbase/regionserver/DefaultMemStore.java | 50 +++++++++------
.../hbase/regionserver/StoreFileScanner.java | 67 +++++++++++---------
.../hadoop/hbase/regionserver/TestHRegion.java | 58 +++++++++++++++++
3 files changed, 124 insertions(+), 51 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/hbase/blob/65345836/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/DefaultMemStore.java
----------------------------------------------------------------------
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/DefaultMemStore.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/DefaultMemStore.java
index cc8c3a8..9bc6a9c 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/DefaultMemStore.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/DefaultMemStore.java
@@ -854,27 +854,35 @@ public class DefaultMemStore implements MemStore {
* specified key, then seek to the first KeyValue of previous row
*/
@Override
- public synchronized boolean seekToPreviousRow(Cell key) {
- Cell firstKeyOnRow = CellUtil.createFirstOnRow(key);
- SortedSet<Cell> cellHead = cellSetAtCreation.headSet(firstKeyOnRow);
- Cell cellSetBeforeRow = cellHead.isEmpty() ? null : cellHead.last();
- SortedSet<Cell> snapshotHead = snapshotAtCreation
- .headSet(firstKeyOnRow);
- Cell snapshotBeforeRow = snapshotHead.isEmpty() ? null : snapshotHead
- .last();
- Cell lastCellBeforeRow = getHighest(cellSetBeforeRow, snapshotBeforeRow);
- if (lastCellBeforeRow == null) {
- theNext = null;
- return false;
- }
- Cell firstKeyOnPreviousRow = CellUtil.createFirstOnRow(lastCellBeforeRow);
- this.stopSkippingCellsIfNextRow = true;
- seek(firstKeyOnPreviousRow);
- this.stopSkippingCellsIfNextRow = false;
- if (peek() == null
- || comparator.compareRows(peek(), firstKeyOnPreviousRow) > 0) {
- return seekToPreviousRow(lastCellBeforeRow);
- }
+ public synchronized boolean seekToPreviousRow(Cell originalKey) {
+ boolean keepSeeking = false;
+ Cell key = originalKey;
+ do {
+ Cell firstKeyOnRow = CellUtil.createFirstOnRow(key);
+ SortedSet<Cell> cellHead = cellSetAtCreation.headSet(firstKeyOnRow);
+ Cell cellSetBeforeRow = cellHead.isEmpty() ? null : cellHead.last();
+ SortedSet<Cell> snapshotHead = snapshotAtCreation
+ .headSet(firstKeyOnRow);
+ Cell snapshotBeforeRow = snapshotHead.isEmpty() ? null : snapshotHead
+ .last();
+ Cell lastCellBeforeRow = getHighest(cellSetBeforeRow, snapshotBeforeRow);
+ if (lastCellBeforeRow == null) {
+ theNext = null;
+ return false;
+ }
+ Cell firstKeyOnPreviousRow = CellUtil.createFirstOnRow(lastCellBeforeRow);
+ this.stopSkippingCellsIfNextRow = true;
+ seek(firstKeyOnPreviousRow);
+ this.stopSkippingCellsIfNextRow = false;
+ if (peek() == null
+ || comparator.compareRows(peek(), firstKeyOnPreviousRow) > 0) {
+ keepSeeking = true;
+ key = firstKeyOnPreviousRow;
+ continue;
+ } else {
+ keepSeeking = false;
+ }
+ } while (keepSeeking);
return true;
}
http://git-wip-us.apache.org/repos/asf/hbase/blob/65345836/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java
----------------------------------------------------------------------
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java
index 108b889..f9e1a3c 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java
@@ -428,37 +428,44 @@ public class StoreFileScanner implements KeyValueScanner {
}
@Override
- public boolean seekToPreviousRow(Cell key) throws IOException {
+ public boolean seekToPreviousRow(Cell originalKey) throws IOException {
try {
try {
- Cell seekKey = CellUtil.createFirstOnRow(key);
- if (seekCount != null) seekCount.incrementAndGet();
- if (!hfs.seekBefore(seekKey)) {
- this.cur = null;
- return false;
- }
- Cell curCell = hfs.getCell();
- Cell firstKeyOfPreviousRow = CellUtil.createFirstOnRow(curCell);
-
- if (seekCount != null) seekCount.incrementAndGet();
- if (!seekAtOrAfter(hfs, firstKeyOfPreviousRow)) {
- this.cur = null;
- return false;
- }
-
- setCurrentCell(hfs.getCell());
- this.stopSkippingKVsIfNextRow = true;
- boolean resultOfSkipKVs;
- try {
- resultOfSkipKVs = skipKVsNewerThanReadpoint();
- } finally {
- this.stopSkippingKVsIfNextRow = false;
- }
- if (!resultOfSkipKVs
- || getComparator().compareRows(cur, firstKeyOfPreviousRow) > 0) {
- return seekToPreviousRow(firstKeyOfPreviousRow);
- }
-
+ boolean keepSeeking = false;
+ Cell key = originalKey;
+ do {
+ Cell seekKey = CellUtil.createFirstOnRow(key);
+ if (seekCount != null) seekCount.incrementAndGet();
+ if (!hfs.seekBefore(seekKey)) {
+ this.cur = null;
+ return false;
+ }
+ Cell curCell = hfs.getCell();
+ Cell firstKeyOfPreviousRow = CellUtil.createFirstOnRow(curCell);
+
+ if (seekCount != null) seekCount.incrementAndGet();
+ if (!seekAtOrAfter(hfs, firstKeyOfPreviousRow)) {
+ this.cur = null;
+ return false;
+ }
+
+ setCurrentCell(hfs.getCell());
+ this.stopSkippingKVsIfNextRow = true;
+ boolean resultOfSkipKVs;
+ try {
+ resultOfSkipKVs = skipKVsNewerThanReadpoint();
+ } finally {
+ this.stopSkippingKVsIfNextRow = false;
+ }
+ if (!resultOfSkipKVs
+ || getComparator().compareRows(cur, firstKeyOfPreviousRow) > 0) {
+ keepSeeking = true;
+ key = firstKeyOfPreviousRow;
+ continue;
+ } else {
+ keepSeeking = false;
+ }
+ } while (keepSeeking);
return true;
} finally {
realSeekDone = true;
@@ -467,7 +474,7 @@ public class StoreFileScanner implements KeyValueScanner {
throw e;
} catch (IOException ioe) {
throw new IOException("Could not seekToPreviousRow " + this + " to key "
- + key, ioe);
+ + originalKey, ioe);
}
}
http://git-wip-us.apache.org/repos/asf/hbase/blob/65345836/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java
----------------------------------------------------------------------
diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java
index e1e11d1..ed45c2d 100644
--- a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java
+++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java
@@ -5774,6 +5774,64 @@ public class TestHRegion {
}
}
+ /**
+ * Test for HBASE-14497: Reverse Scan threw StackOverflow caused by readPt checking
+ */
+ @Test (timeout = 60000)
+ public void testReverseScanner_StackOverflow() throws IOException {
+ byte[] cf1 = Bytes.toBytes("CF1");
+ byte[][] families = {cf1};
+ byte[] col = Bytes.toBytes("C");
+ String method = this.getName();
+ HBaseConfiguration conf = new HBaseConfiguration();
+ this.region = initHRegion(tableName, method, conf, families);
+ try {
+ // setup with one storefile and one memstore, to create scanner and get an earlier readPt
+ Put put = new Put(Bytes.toBytes("19998"));
+ put.add(cf1, col, Bytes.toBytes("val"));
+ region.put(put);
+ region.flushcache(true, true);
+ Put put2 = new Put(Bytes.toBytes("19997"));
+ put2.add(cf1, col, Bytes.toBytes("val"));
+ region.put(put2);
+
+ Scan scan = new Scan(Bytes.toBytes("19998"));
+ scan.setReversed(true);
+ InternalScanner scanner = region.getScanner(scan);
+
+ // create one storefile contains many rows will be skipped
+ // to check StoreFileScanner.seekToPreviousRow
+ for (int i = 10000; i < 20000; i++) {
+ Put p = new Put(Bytes.toBytes(""+i));
+ p.add(cf1, col, Bytes.toBytes(""+i));
+ region.put(p);
+ }
+ region.flushcache(true, true);
+
+ // create one memstore contains many rows will be skipped
+ // to check MemStoreScanner.seekToPreviousRow
+ for (int i = 10000; i < 20000; i++) {
+ Put p = new Put(Bytes.toBytes(""+i));
+ p.add(cf1, col, Bytes.toBytes(""+i));
+ region.put(p);
+ }
+
+ List<Cell> currRow = new ArrayList<>();
+ boolean hasNext;
+ do {
+ hasNext = scanner.next(currRow);
+ } while (hasNext);
+ assertEquals(2, currRow.size());
+ assertEquals("19998", Bytes.toString(currRow.get(0).getRowArray(),
+ currRow.get(0).getRowOffset(), currRow.get(0).getRowLength()));
+ assertEquals("19997", Bytes.toString(currRow.get(1).getRowArray(),
+ currRow.get(1).getRowOffset(), currRow.get(1).getRowLength()));
+ } finally {
+ HBaseTestingUtility.closeRegionAndWAL(this.region);
+ this.region = null;
+ }
+ }
+
@Test (timeout=60000)
public void testSplitRegionWithReverseScan() throws IOException {
TableName tableName = TableName.valueOf("testSplitRegionWithReverseScan");