HBASE-14497 Reverse Scan threw StackOverflow caused by readPt checking (Yerui Sun)

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/DefaultMemStore.java

@@ -969,29 +969,37 @@ public class DefaultMemStore implements MemStore {
      * specified key, then seek to the first KeyValue of previous row
      */
     @Override
-    public synchronized boolean seekToPreviousRow(Cell key) {
+    public synchronized boolean seekToPreviousRow(Cell originalKey) {
-      Cell firstKeyOnRow = KeyValueUtil.createFirstOnRow(key.getRowArray(), key.getRowOffset(),
+      boolean keepSeeking = false;
-          key.getRowLength());
+      Cell key = originalKey;
-      SortedSet<Cell> cellHead = cellSetAtCreation.headSet(firstKeyOnRow);
+      do {
-      Cell cellSetBeforeRow = cellHead.isEmpty() ? null : cellHead.last();
+        Cell firstKeyOnRow = KeyValueUtil.createFirstOnRow(key.getRowArray(), key.getRowOffset(),
-      SortedSet<Cell> snapshotHead = snapshotAtCreation
+            key.getRowLength());
-          .headSet(firstKeyOnRow);
+        SortedSet<Cell> cellHead = cellSetAtCreation.headSet(firstKeyOnRow);
-      Cell snapshotBeforeRow = snapshotHead.isEmpty() ? null : snapshotHead
+        Cell cellSetBeforeRow = cellHead.isEmpty() ? null : cellHead.last();
-          .last();
+        SortedSet<Cell> snapshotHead = snapshotAtCreation
-      Cell lastCellBeforeRow = getHighest(cellSetBeforeRow, snapshotBeforeRow);
+            .headSet(firstKeyOnRow);
-      if (lastCellBeforeRow == null) {
+        Cell snapshotBeforeRow = snapshotHead.isEmpty() ? null : snapshotHead
-        theNext = null;
+            .last();
-        return false;
+        Cell lastCellBeforeRow = getHighest(cellSetBeforeRow, snapshotBeforeRow);
-      }
+        if (lastCellBeforeRow == null) {
-      Cell firstKeyOnPreviousRow = KeyValueUtil.createFirstOnRow(lastCellBeforeRow.getRowArray(),
+          theNext = null;
-          lastCellBeforeRow.getRowOffset(), lastCellBeforeRow.getRowLength());
+          return false;
-      this.stopSkippingCellsIfNextRow = true;
+        }
-      seek(firstKeyOnPreviousRow);
+        Cell firstKeyOnPreviousRow = KeyValueUtil.createFirstOnRow(lastCellBeforeRow.getRowArray(),
-      this.stopSkippingCellsIfNextRow = false;
+            lastCellBeforeRow.getRowOffset(), lastCellBeforeRow.getRowLength());
-      if (peek() == null
+        this.stopSkippingCellsIfNextRow = true;
-          || comparator.compareRows(peek(), firstKeyOnPreviousRow) > 0) {
+        seek(firstKeyOnPreviousRow);
-        return seekToPreviousRow(lastCellBeforeRow);
+        this.stopSkippingCellsIfNextRow = false;
-      }
+        if (peek() == null
+            || comparator.compareRows(peek(), firstKeyOnPreviousRow) > 0) {
+          keepSeeking = true;
+          key = firstKeyOnPreviousRow;
+          continue;
+        } else {
+          keepSeeking = false;
+        }
+      } while (keepSeeking);
       return true;
     }
 

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java

@@ -437,39 +437,46 @@ public class StoreFileScanner implements KeyValueScanner {
 
   @Override
   @SuppressWarnings("deprecation")
-  public boolean seekToPreviousRow(Cell key) throws IOException {
+  public boolean seekToPreviousRow(Cell originalKey) throws IOException {
     try {
       try {
-        KeyValue seekKey = KeyValueUtil.createFirstOnRow(key.getRowArray(), key.getRowOffset(),
+        boolean keepSeeking = false;
-            key.getRowLength());
+        Cell key = originalKey;
-        if (seekCount != null) seekCount.incrementAndGet();
+        do {
-        if (!hfs.seekBefore(seekKey.getBuffer(), seekKey.getKeyOffset(),
+          KeyValue seekKey = KeyValueUtil.createFirstOnRow(key.getRowArray(), key.getRowOffset(),
-            seekKey.getKeyLength())) {
+              key.getRowLength());
-          close();
+          if (seekCount != null) seekCount.incrementAndGet();
-          return false;
+          if (!hfs.seekBefore(seekKey.getBuffer(), seekKey.getKeyOffset(),
-        }
+              seekKey.getKeyLength())) {
-        KeyValue firstKeyOfPreviousRow = KeyValueUtil.createFirstOnRow(hfs.getKeyValue()
+            close();
-            .getRowArray(), hfs.getKeyValue().getRowOffset(), hfs.getKeyValue().getRowLength());
+            return false;
+          }
+          KeyValue firstKeyOfPreviousRow = KeyValueUtil.createFirstOnRow(hfs.getKeyValue()
+              .getRowArray(), hfs.getKeyValue().getRowOffset(), hfs.getKeyValue().getRowLength());
 
-        if (seekCount != null) seekCount.incrementAndGet();
+          if (seekCount != null) seekCount.incrementAndGet();
-        if (!seekAtOrAfter(hfs, firstKeyOfPreviousRow)) {
+          if (!seekAtOrAfter(hfs, firstKeyOfPreviousRow)) {
-          close();
+            close();
-          return false;
+            return false;
-        }
+          }
-
-        setCurrentCell(hfs.getKeyValue());
-        this.stopSkippingKVsIfNextRow = true;
-        boolean resultOfSkipKVs;
-        try {
-          resultOfSkipKVs = skipKVsNewerThanReadpoint();
-        } finally {
-          this.stopSkippingKVsIfNextRow = false;
-        }
-        if (!resultOfSkipKVs
-            || getComparator().compareRows(cur, firstKeyOfPreviousRow) > 0) {
-          return seekToPreviousRow(firstKeyOfPreviousRow);
-        }
 
+          setCurrentCell(hfs.getKeyValue());
+          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;
@@ -478,7 +485,7 @@ public class StoreFileScanner implements KeyValueScanner {
       throw e;
     } catch (IOException ioe) {
       throw new IOException("Could not seekToPreviousRow " + this + " to key "
-          + key, ioe);
+          + originalKey, ioe);
     }
   }
 

hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java

@@ -5748,6 +5748,62 @@ 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());
+      assertArrayEquals(Bytes.toBytes("19998"), currRow.get(0).getRow());
+      assertArrayEquals(Bytes.toBytes("19997"), currRow.get(1).getRow());
+    } finally {
+      HBaseTestingUtility.closeRegionAndWAL(this.region);
+      this.region = null;
+    }
+  }
+
   @Test (timeout=60000)
   public void testSplitRegionWithReverseScan() throws IOException {
     byte [] tableName = Bytes.toBytes("testSplitRegionWithReverseScan");