.../apache/hadoop/hbase/regionserver/HStore.java | 12 ++-- .../apache/hadoop/hbase/regionserver/Store.java | 1 + .../hadoop/hbase/regionserver/StoreScanner.java | 22 +++++-- .../apache/hadoop/hbase/HBaseTestingUtility.java | 41 ------------- .../hadoop/hbase/regionserver/TestHRegion.java | 58 ++++++++++++++++++ .../hbase/regionserver/TestScanWithBloomError.java | 2 +- .../hadoop/hbase/regionserver/TestStore.java | 68 +++++++++++++++++----- 7 files changed, 138 insertions(+), 66 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java index b598f09..d5e2e5f 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java @@ -1127,12 +1127,14 @@ public class HStore implements Store { boolean usePread, boolean isCompaction, ScanQueryMatcher matcher, byte[] startRow, byte[] stopRow, long readPt) throws IOException { Collection storeFilesToScan; - List memStoreScanners; + List memStoreScanners = null; this.lock.readLock().lock(); try { storeFilesToScan = this.storeEngine.getStoreFileManager().getFilesForScanOrGet(isGet, startRow, stopRow); - memStoreScanners = this.memstore.getScanners(readPt); + if (readPt >= this.getMaxSequenceId()) { + memStoreScanners = this.memstore.getScanners(readPt); + } } finally { this.lock.readLock().unlock(); } @@ -1148,7 +1150,9 @@ public class HStore implements Store { new ArrayList(sfScanners.size()+1); scanners.addAll(sfScanners); // Then the memstore scanners - scanners.addAll(memStoreScanners); + if (memStoreScanners != null) { + scanners.addAll(memStoreScanners); + } return scanners; } @@ -1157,7 +1161,7 @@ public class HStore implements Store { boolean isGet, boolean usePread, boolean isCompaction, ScanQueryMatcher matcher, byte[] startRow, byte[] stopRow, long readPt, boolean includeMemstoreScanner) throws IOException { List memStoreScanners = null; - if (includeMemstoreScanner) { + if (includeMemstoreScanner && (readPt >= this.getMaxSequenceId())) { this.lock.readLock().lock(); try { memStoreScanners = this.memstore.getScanners(readPt); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java index 1b41eb0..8c108f3 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java @@ -96,6 +96,7 @@ public interface Store extends HeapSize, StoreConfigInformation, PropagatingConf * @param readPt * @return all scanners for this store */ + @Deprecated List getScanners( boolean cacheBlocks, boolean isGet, diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java index c98af00..9239554 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java @@ -346,8 +346,8 @@ public class StoreScanner extends NonReversedNonLazyKeyValueScanner protected List getScannersNoCompaction() throws IOException { final boolean isCompaction = false; boolean usePread = get || scanUsePread; - return selectScannersFrom(store.getScanners(cacheBlocks, get, usePread, - isCompaction, matcher, scan.getStartRow(), scan.getStopRow(), this.readPt)); + return selectScannersFrom(store.getScanners(null, cacheBlocks, get, usePread, + isCompaction, matcher, scan.getStartRow(), scan.getStopRow(), this.readPt, true)); } /** @@ -977,13 +977,23 @@ public class StoreScanner extends NonReversedNonLazyKeyValueScanner * Used in testing. * @return all scanners in no particular order */ + @VisibleForTesting List getAllScannersForTesting() { List allScanners = new ArrayList(); - KeyValueScanner current = heap.getCurrentForTesting(); - if (current != null) - allScanners.add(current); - for (KeyValueScanner scanner : heap.getHeap()) + for (KeyValueScanner scanner : currentScanners) { allScanners.add(scanner); + } + return allScanners; + } + + @VisibleForTesting + List getAllStoreFileScannersForTesting() { + List allScanners = new ArrayList(); + for (KeyValueScanner scanner : currentScanners) { + if (scanner.isFileScanner()) { + allScanners.add(scanner); + } + } return allScanners; } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java index 4547e0f..aaeadbb 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java @@ -3479,32 +3479,6 @@ public class HBaseTestingUtility extends HBaseCommonTestingUtility { } /** - * Do a small get/scan against one store. This is required because store - * has no actual methods of querying itself, and relies on StoreScanner. - */ - public static List getFromStoreFile(HStore store, - Get get) throws IOException { - Scan scan = new Scan(get); - InternalScanner scanner = (InternalScanner) store.getScanner(scan, - scan.getFamilyMap().get(store.getFamily().getName()), - // originally MultiVersionConcurrencyControl.resetThreadReadPoint() was called to set - // readpoint 0. - 0); - - List result = new ArrayList(); - scanner.next(result); - if (!result.isEmpty()) { - // verify that we are on the row we want: - Cell kv = result.get(0); - if (!CellUtil.matchingRow(kv, get.getRow())) { - result.clear(); - } - } - scanner.close(); - return result; - } - - /** * Create region split keys between startkey and endKey * * @param startKey @@ -3522,21 +3496,6 @@ public class HBaseTestingUtility extends HBaseCommonTestingUtility { } /** - * Do a small get/scan against one store. This is required because store - * has no actual methods of querying itself, and relies on StoreScanner. - */ - public static List getFromStoreFile(HStore store, - byte [] row, - NavigableSet columns - ) throws IOException { - Get get = new Get(row); - Map> s = get.getFamilyMap(); - s.put(store.getFamily().getName(), columns); - - return getFromStoreFile(store,get); - } - - /** * Gets a ZooKeeperWatcher. * @param TEST_UTIL */ 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 de844ba..f7a4759 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 @@ -5869,6 +5869,64 @@ public class TestHRegion { } } + @Test(timeout = 60000) + public void testReverseScanShouldNotScanMemstoreIfReadPtlesser() throws Exception { + 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("19996")); + put.addColumn(cf1, col, Bytes.toBytes("val")); + region.put(put); + Put put2 = new Put(Bytes.toBytes("19995")); + put2.addColumn(cf1, col, Bytes.toBytes("val")); + region.put(put2); + // create a reverse scan + Scan scan = new Scan(Bytes.toBytes("19996")); + scan.setReversed(true); + RegionScanner scanner = region.getScanner(scan); + + // flush the cache. This will reset the store scanner + 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.addColumn(cf1, col, Bytes.toBytes("" + i)); + region.put(p); + } + List currRow = new ArrayList<>(); + boolean hasNext; + boolean assertDone = false; + do { + hasNext = scanner.next(currRow); + // With HBASE-15871, after the scanner is reset the memstore scanner should not be + // added here + if (!assertDone) { + StoreScanner current = + (StoreScanner) (((RegionScannerImpl) scanner).storeHeap).getCurrentForTesting(); + List scanners = current.getAllScannersForTesting(); + assertEquals("There should be only one scanner the store file scanner", 1, + scanners.size()); + assertDone = true; + } + } while (hasNext); + assertEquals(2, currRow.size()); + assertEquals("19996", Bytes.toString(currRow.get(0).getRowArray(), + currRow.get(0).getRowOffset(), currRow.get(0).getRowLength())); + assertEquals("19995", 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 { byte [] tableName = Bytes.toBytes("testSplitRegionWithReverseScan"); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestScanWithBloomError.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestScanWithBloomError.java index 7682024..232942e 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestScanWithBloomError.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestScanWithBloomError.java @@ -127,7 +127,7 @@ public class TestScanWithBloomError { (StoreScanner) storeHeap.getCurrentForTesting(); @SuppressWarnings({ "unchecked", "rawtypes" }) List scanners = (List) - (List) storeScanner.getAllScannersForTesting(); + (List) storeScanner.getAllStoreFileScannersForTesting(); // Sort scanners by their HFile's modification time. Collections.sort(scanners, new Comparator() { diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java index 414c663..d71a635 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java @@ -33,6 +33,7 @@ import java.util.Collection; import java.util.Collections; import java.util.Iterator; import java.util.List; +import java.util.Map; import java.util.NavigableSet; import java.util.concurrent.ConcurrentSkipListSet; import java.util.concurrent.atomic.AtomicBoolean; @@ -60,6 +61,7 @@ import org.apache.hadoop.hbase.KeyValueUtil; import org.apache.hadoop.hbase.testclassification.MediumTests; import org.apache.hadoop.hbase.TableName; import org.apache.hadoop.hbase.client.Get; +import org.apache.hadoop.hbase.client.Scan; import org.apache.hadoop.hbase.io.compress.Compression; import org.apache.hadoop.hbase.io.encoding.DataBlockEncoding; import org.apache.hadoop.hbase.io.hfile.CacheConfig; @@ -118,7 +120,7 @@ public class TestStore { List expected = new ArrayList(); List result = new ArrayList(); - long id = System.currentTimeMillis(); + static long id = 1; Get get = new Get(row); private HBaseTestingUtility TEST_UTIL = new HBaseTestingUtility(); @@ -444,7 +446,7 @@ public class TestStore { this.store = new HStore(this.store.getHRegion(), this.store.getFamily(), c); Assert.assertEquals(2, this.store.getStorefilesCount()); - result = HBaseTestingUtility.getFromStoreFile(store, + result = getFromStoreFile(store, get.getRow(), qualifiers); Assert.assertEquals(1, result.size()); @@ -467,7 +469,7 @@ public class TestStore { this.store.add(new KeyValue(row, family, qf6, 1, (byte[])null)); //Get - result = HBaseTestingUtility.getFromStoreFile(store, + result = getFromStoreFile(store, get.getRow(), qualifiers); //Compare @@ -501,7 +503,7 @@ public class TestStore { flush(3); //Get - result = HBaseTestingUtility.getFromStoreFile(store, + result = getFromStoreFile(store, get.getRow(), qualifiers); //this.store.get(get, qualifiers, result); @@ -538,7 +540,7 @@ public class TestStore { this.store.add(new KeyValue(row, family, qf6, 1, (byte[])null)); //Get - result = HBaseTestingUtility.getFromStoreFile(store, + result = getFromStoreFile(store, get.getRow(), qualifiers); //Need to sort the result since multiple files @@ -548,6 +550,44 @@ public class TestStore { assertCheck(); } + /** + * Do a small get/scan against one store. This is required because store has no actual methods of + * querying itself, and relies on StoreScanner. + */ + private static List getFromStoreFile(HStore store, byte[] row, NavigableSet columns) + throws IOException { + Get get = new Get(row); + Map> s = get.getFamilyMap(); + s.put(store.getFamily().getName(), columns); + + return getFromStoreFile(store, get); + } + + /** + * Do a small get/scan against one store. This is required because store has no actual methods of + * querying itself, and relies on StoreScanner. + */ + private static List getFromStoreFile(HStore store, Get get) throws IOException { + Scan scan = new Scan(get); + InternalScanner scanner = (InternalScanner) store.getScanner(scan, + scan.getFamilyMap().get(store.getFamily().getName()), + // originally MultiVersionConcurrencyControl.resetThreadReadPoint() was called to set + // readpoint 0. + id++); + + List result = new ArrayList(); + scanner.next(result); + if (!result.isEmpty()) { + // verify that we are on the row we want: + Cell kv = result.get(0); + if (!CellUtil.matchingRow(kv, get.getRow())) { + result.clear(); + } + } + scanner.close(); + return result; + } + private void flush(int storeFilessize) throws IOException{ this.store.snapshot(); flushStore(store, id++); @@ -605,7 +645,7 @@ public class TestStore { get.setMaxVersions(); // all versions. List results = new ArrayList(); - results = HBaseTestingUtility.getFromStoreFile(store, get); + results = getFromStoreFile(store, get); Assert.assertEquals(2, results.size()); long ts1 = results.get(0).getTimestamp(); @@ -717,7 +757,7 @@ public class TestStore { get.setMaxVersions(); // all versions. List results = new ArrayList(); - results = HBaseTestingUtility.getFromStoreFile(store, get); + results = getFromStoreFile(store, get); Assert.assertEquals(2, results.size()); long ts1 = results.get(0).getTimestamp(); @@ -731,7 +771,7 @@ public class TestStore { newValue += 1; this.store.updateColumnValue(row, family, qf1, newValue); - results = HBaseTestingUtility.getFromStoreFile(store, get); + results = getFromStoreFile(store, get); Assert.assertEquals(2, results.size()); ts1 = results.get(0).getTimestamp(); @@ -912,27 +952,27 @@ public class TestStore { get.addColumn(family,qf1); get.setTimeRange(0,15); - result = HBaseTestingUtility.getFromStoreFile(store, get); + result = getFromStoreFile(store, get); Assert.assertTrue(result.size()>0); get.setTimeRange(40,90); - result = HBaseTestingUtility.getFromStoreFile(store, get); + result = getFromStoreFile(store, get); Assert.assertTrue(result.size()>0); get.setTimeRange(10,45); - result = HBaseTestingUtility.getFromStoreFile(store, get); + result = getFromStoreFile(store, get); Assert.assertTrue(result.size()>0); get.setTimeRange(80,145); - result = HBaseTestingUtility.getFromStoreFile(store, get); + result = getFromStoreFile(store, get); Assert.assertTrue(result.size()>0); get.setTimeRange(1,2); - result = HBaseTestingUtility.getFromStoreFile(store, get); + result = getFromStoreFile(store, get); Assert.assertTrue(result.size()>0); get.setTimeRange(90,200); - result = HBaseTestingUtility.getFromStoreFile(store, get); + result = getFromStoreFile(store, get); Assert.assertTrue(result.size()==0); }