From f83490b7b7be829bf03dd9af0c87b74eb109b69d Mon Sep 17 00:00:00 2001 From: sunyerui Date: Tue, 29 Sep 2015 14:05:12 +0800 Subject: [PATCH] HBASE-14497 Reverse Scan threw StackOverflow caused by readPt checking --- .../hadoop/hbase/regionserver/DefaultMemStore.java | 50 +++++++++------- .../hbase/regionserver/StoreFileScanner.java | 67 ++++++++++++---------- .../hadoop/hbase/regionserver/TestHRegion.java | 52 +++++++++++++++++ 3 files changed, 118 insertions(+), 51 deletions(-) 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 cellHead = cellSetAtCreation.headSet(firstKeyOnRow); - Cell cellSetBeforeRow = cellHead.isEmpty() ? null : cellHead.last(); - SortedSet 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 cellHead = cellSetAtCreation.headSet(firstKeyOnRow); + Cell cellSetBeforeRow = cellHead.isEmpty() ? null : cellHead.last(); + SortedSet 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; } 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); } } 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 cb95d6f..fa5d6fa 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 @@ -5776,6 +5776,58 @@ 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); + + // 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 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())); + } + @Test (timeout=60000) public void testSplitRegionWithReverseScan() throws IOException { TableName tableName = TableName.valueOf("testSplitRegionWithReverseScan"); -- 2.3.2 (Apple Git-55)