diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueScanner.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueScanner.java index eae713f..a278472 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueScanner.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueScanner.java @@ -29,6 +29,9 @@ import org.apache.hadoop.hbase.client.Scan; * Scanner that returns the next KeyValue. */ @InterfaceAudience.Private +// TODO: Change name from KeyValueScanner to CellScanner only we already have a simple CellScanner +// so this should be something else altogether, a decoration on our base CellScanner. TODO. +// This class shows in CPs so do it all in one swell swoop. HBase-2.0.0. public interface KeyValueScanner extends Shipper { /** * The byte array represents for NO_NEXT_INDEXED_KEY; diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java index c220b5c..ff23a0e 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java @@ -93,7 +93,7 @@ public class ScanQueryMatcher { /* row is not private for tests */ /** Row the query is on */ Cell curCell; - + /** * Oldest put in any of the involved store files * Used to decide whether it is ok to delete @@ -119,7 +119,7 @@ public class ScanQueryMatcher { * first column. * */ private boolean hasNullColumn = true; - + private RegionCoprocessorHost regionCoprocessorHost= null; // By default, when hbase.hstore.time.to.purge.deletes is 0ms, a delete @@ -140,12 +140,17 @@ public class ScanQueryMatcher { // currently influencing. This is because Puts, that this delete can // influence. may appear out of order. private final long timeToPurgeDeletes; - + private final boolean isUserScan; private final boolean isReversed; /** + * True if we are doing a 'Get' Scan. Every Get is actually a one-row Scan. + */ + private final boolean get; + + /** * Construct a QueryMatcher for a scan * @param scan * @param scanInfo The store's immutable scan info @@ -154,8 +159,8 @@ public class ScanQueryMatcher { * @param earliestPutTs Earliest put seen in any of the store files. * @param oldestUnexpiredTS the oldest timestamp we are interested in, * based on TTL - * @param regionCoprocessorHost - * @throws IOException + * @param regionCoprocessorHost + * @throws IOException */ public ScanQueryMatcher(Scan scan, ScanInfo scanInfo, NavigableSet columns, ScanType scanType, long readPointToUse, long earliestPutTs, long oldestUnexpiredTS, @@ -166,6 +171,7 @@ public class ScanQueryMatcher { } else { this.tr = timeRange; } + this.get = scan.isGetScan(); this.rowComparator = scanInfo.getComparator(); this.regionCoprocessorHost = regionCoprocessorHost; this.deletes = instantiateDeleteTracker(); @@ -234,8 +240,8 @@ public class ScanQueryMatcher { * @param now the current server time * @param dropDeletesFromRow The inclusive left bound of the range; can be EMPTY_START_ROW. * @param dropDeletesToRow The exclusive right bound of the range; can be EMPTY_END_ROW. - * @param regionCoprocessorHost - * @throws IOException + * @param regionCoprocessorHost + * @throws IOException */ public ScanQueryMatcher(Scan scan, ScanInfo scanInfo, NavigableSet columns, long readPointToUse, long earliestPutTs, long oldestUnexpiredTS, long now, @@ -280,7 +286,7 @@ public class ScanQueryMatcher { * caused by a data corruption. */ public MatchCode match(Cell cell) throws IOException { - if (filter != null && filter.filterAllRemaining()) { + if (filter != null && filter.filterAllRemaining()) { return MatchCode.DONE_SCAN; } if (curCell != null) { @@ -324,7 +330,7 @@ public class ScanQueryMatcher { // check if the cell is expired by cell TTL if (HStore.isCellTTLExpired(cell, this.oldestUnexpiredTS, this.now)) { return MatchCode.SKIP; - } + } /* * The delete logic is pretty complicated now. @@ -359,10 +365,10 @@ public class ScanQueryMatcher { } // Can't early out now, because DelFam come before any other keys } - + if ((!isUserScan) && timeToPurgeDeletes > 0 - && (EnvironmentEdgeManager.currentTime() - timestamp) + && (EnvironmentEdgeManager.currentTime() - timestamp) <= timeToPurgeDeletes) { return MatchCode.INCLUDE; } else if (retainDeletesInOutput || mvccVersion > maxReadPointToTrackVersions) { @@ -503,22 +509,26 @@ public class ScanQueryMatcher { } } + /** + * @return Returns false if we know there are no more rows to be scanned (We've reached the + * stopRow or we are scanning on row only because this Scan is for a Get, etc. + */ public boolean moreRowsMayExistAfter(Cell kv) { - if (this.isReversed) { - if (rowComparator.compareRows(kv, stopRow, 0, stopRow.length) <= 0) { - return false; - } else { - return true; - } - } - if (!Bytes.equals(stopRow , HConstants.EMPTY_END_ROW) && - rowComparator.compareRows(kv, stopRow, 0, stopRow.length) >= 0) { - // KV >= STOPROW - // then NO there is nothing left. + // If a 'get' Scan -- we are doing a Get (every Get is a single-row Scan in implementation) -- + // then we are looking at one row only, the one specified in the Get coordinate..so we know + // for sure that there are no more rows on this Scan + if (this.get) { return false; - } else { + } + // If no stopRow, return that may be more rows. The tests that follow depend on a non-empty + // stopRow so this little compare short-circuits + if (this.stopRow == null || this.stopRow == HConstants.EMPTY_BYTE_ARRAY) { return true; } + return this.isReversed? + rowComparator.compareRows(kv, stopRow, 0, stopRow.length) > 0: + Bytes.equals(stopRow, HConstants.EMPTY_END_ROW) || + rowComparator.compareRows(kv, stopRow, 0, stopRow.length) < 0; } /** 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 2f0d284..0f60acb 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 @@ -650,10 +650,10 @@ public class StoreScanner extends NonReversedNonLazyKeyValueScanner } /* - * See if we should actually SEEK or rather just SKIP to the next Cell. - * (see HBASE-13109) + * See if we should actually SEEK or rather just SKIP to the next Cell (see HBASE-13109) */ - private ScanQueryMatcher.MatchCode optimize(ScanQueryMatcher.MatchCode qcode, Cell cell) { + @VisibleForTesting + protected ScanQueryMatcher.MatchCode optimize(ScanQueryMatcher.MatchCode qcode, Cell cell) { switch(qcode) { case INCLUDE_AND_SEEK_NEXT_COL: case SEEK_NEXT_COL: @@ -668,6 +668,9 @@ public class StoreScanner extends NonReversedNonLazyKeyValueScanner case INCLUDE_AND_SEEK_NEXT_ROW: case SEEK_NEXT_ROW: { + if (!this.matcher.moreRowsMayExistAfter(cell)) { + return qcode; + } Cell nextIndexedKey = getNextIndexedKey(); if (nextIndexedKey != null && nextIndexedKey != KeyValueScanner.NO_NEXT_INDEXED_KEY && matcher.compareKeyForNextRow(nextIndexedKey, cell) >= 0) { diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/util/CollectionBackedScanner.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/util/CollectionBackedScanner.java index 9fc068f..4720880 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/util/CollectionBackedScanner.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/util/CollectionBackedScanner.java @@ -30,8 +30,7 @@ import org.apache.hadoop.hbase.CellComparator; import org.apache.hadoop.hbase.regionserver.NonReversedNonLazyKeyValueScanner; /** - * Utility scanner that wraps a sortable collection and serves - * as a KeyValueScanner. + * Utility scanner that wraps a sortable collection and serves as a KeyValueScanner. */ @InterfaceAudience.Private public class CollectionBackedScanner extends NonReversedNonLazyKeyValueScanner { diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/KeyValueScanFixture.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/KeyValueScanFixture.java index 3f87a00..a4e7f9b 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/KeyValueScanFixture.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/KeyValueScanFixture.java @@ -22,6 +22,7 @@ package org.apache.hadoop.hbase.regionserver; import java.util.ArrayList; import java.util.List; +import org.apache.hadoop.hbase.Cell; import org.apache.hadoop.hbase.CellComparator; import org.apache.hadoop.hbase.KeyValue; import org.apache.hadoop.hbase.util.CollectionBackedScanner; @@ -33,9 +34,8 @@ import org.apache.hadoop.hbase.util.CollectionBackedScanner; * to be a store file scanner. */ public class KeyValueScanFixture extends CollectionBackedScanner { - public KeyValueScanFixture(CellComparator comparator, - KeyValue... incData) { - super(comparator, incData); + public KeyValueScanFixture(CellComparator comparator, Cell... cells) { + super(comparator, cells); } public static List scanFixture(KeyValue[] ... kvArrays) { @@ -45,4 +45,4 @@ public class KeyValueScanFixture extends CollectionBackedScanner { } return scanners; } -} +} \ No newline at end of file diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestKeyValueScanFixture.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestKeyValueScanFixture.java index a8c2c65..0e96682 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestKeyValueScanFixture.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestKeyValueScanFixture.java @@ -46,8 +46,7 @@ public class TestKeyValueScanFixture extends TestCase { KeyValueTestUtil.create("RowB", "family", "qf1", 10, KeyValue.Type.Put, "value-10") }; - KeyValueScanner scan = new KeyValueScanFixture( - CellComparator.COMPARATOR, kvs); + KeyValueScanner scan = new KeyValueScanFixture(CellComparator.COMPARATOR, kvs); KeyValue kv = KeyValueUtil.createFirstOnRow(Bytes.toBytes("RowA")); // should seek to this: diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java index 92c85aa..abc8af1 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java @@ -27,16 +27,21 @@ import java.util.Arrays; import java.util.List; import java.util.NavigableSet; import java.util.TreeSet; +import java.util.concurrent.atomic.AtomicInteger; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hbase.CategoryBasedTimeout; import org.apache.hadoop.hbase.Cell; import org.apache.hadoop.hbase.CellComparator; +import org.apache.hadoop.hbase.CellUtil; import org.apache.hadoop.hbase.HBaseConfiguration; import org.apache.hadoop.hbase.HConstants; import org.apache.hadoop.hbase.KeepDeletedCells; import org.apache.hadoop.hbase.KeyValue; import org.apache.hadoop.hbase.KeyValueTestUtil; +import org.apache.hadoop.hbase.client.Get; import org.apache.hadoop.hbase.client.Scan; import org.apache.hadoop.hbase.testclassification.MediumTests; import org.apache.hadoop.hbase.testclassification.RegionServerTests; @@ -53,15 +58,43 @@ import org.junit.rules.TestRule; // Can't be small as it plays with EnvironmentEdgeManager @Category({RegionServerTests.class, MediumTests.class}) public class TestStoreScanner { + private static final Log LOG = LogFactory.getLog(TestStoreScanner.class); @Rule public TestName name = new TestName(); @Rule public final TestRule timeout = CategoryBasedTimeout.builder().withTimeout(this.getClass()). withLookingForStuckThread(true).build(); private static final String CF_STR = "cf"; - final byte [] CF = Bytes.toBytes(CF_STR); + private static final byte [] CF = Bytes.toBytes(CF_STR); static Configuration CONF = HBaseConfiguration.create(); private ScanInfo scanInfo = new ScanInfo(CONF, CF, 0, Integer.MAX_VALUE, Long.MAX_VALUE, KeepDeletedCells.FALSE, 0, CellComparator.COMPARATOR); private ScanType scanType = ScanType.USER_SCAN; + private static final byte [] R1 = new byte [] {'1'}; + private static final byte [] R2 = new byte [] {'2'}; + private static final byte [] R3 = new byte [] {'3'}; + private static final byte [] R4 = new byte [] {'4'}; + private static final byte [] VALUE = new byte [] {'v'}; + /** + * Four rows of four columns distinguished by column qualifier (column qualifier is one of the + * four rows... R1, R2, R3, or R4). + */ + private static final Cell [] CELL_GRID = new Cell [] { + CellUtil.createCell(R1, CF, R1, 1L, KeyValue.Type.Put.getCode(), VALUE), + CellUtil.createCell(R1, CF, R2, 1L, KeyValue.Type.Put.getCode(), VALUE), + CellUtil.createCell(R1, CF, R3, 1L, KeyValue.Type.Put.getCode(), VALUE), + CellUtil.createCell(R1, CF, R4, 1L, KeyValue.Type.Put.getCode(), VALUE), + CellUtil.createCell(R2, CF, R1, 1L, KeyValue.Type.Put.getCode(), VALUE), + CellUtil.createCell(R2, CF, R2, 1L, KeyValue.Type.Put.getCode(), VALUE), + CellUtil.createCell(R2, CF, R3, 1L, KeyValue.Type.Put.getCode(), VALUE), + CellUtil.createCell(R2, CF, R4, 1L, KeyValue.Type.Put.getCode(), VALUE), + CellUtil.createCell(R3, CF, R1, 1L, KeyValue.Type.Put.getCode(), VALUE), + CellUtil.createCell(R3, CF, R2, 1L, KeyValue.Type.Put.getCode(), VALUE), + CellUtil.createCell(R3, CF, R3, 1L, KeyValue.Type.Put.getCode(), VALUE), + CellUtil.createCell(R3, CF, R4, 1L, KeyValue.Type.Put.getCode(), VALUE), + CellUtil.createCell(R4, CF, R1, 1L, KeyValue.Type.Put.getCode(), VALUE), + CellUtil.createCell(R4, CF, R2, 1L, KeyValue.Type.Put.getCode(), VALUE), + CellUtil.createCell(R4, CF, R3, 1L, KeyValue.Type.Put.getCode(), VALUE), + CellUtil.createCell(R4, CF, R4, 1L, KeyValue.Type.Put.getCode(), VALUE), + }; /* * Test utility for building a NavigableSet for scanners. @@ -77,6 +110,150 @@ public class TestStoreScanner { return cols; } + /** + * Test optimize in StoreScanner. Test that we skip to the next 'block' when we it makes sense + * reading the block 'index'. + * @throws IOException + */ + @Test + public void testOptimize() throws IOException { + List scanners = Arrays.asList( + new KeyValueScanner[] {new KeyValueScanFixture(CellComparator.COMPARATOR, CELL_GRID)}); + // First test a Get of the middle Cell in the row R2. Every Get is a Scan. Middle row has a + // qualifier of R2. + Scan scan = new Scan(); + // A scan that just gets the first qualifier on each row -- R1 is first qualifier in our little + // CELL_GRID + scan.addColumn(CF, R1); + // Count of how often optimize is called. + final AtomicInteger count = new AtomicInteger(0); + StoreScanner scanner = + new StoreScanner(scan, scanInfo, scanType, scan.getFamilyMap().get(CF), scanners) { + protected org.apache.hadoop.hbase.regionserver.ScanQueryMatcher.MatchCode + optimize(org.apache.hadoop.hbase.regionserver.ScanQueryMatcher.MatchCode qcode, Cell cell) { + count.incrementAndGet(); + org.apache.hadoop.hbase.regionserver.ScanQueryMatcher.MatchCode after = + super.optimize(qcode, cell); + LOG.info("Cell=" + cell + ", nextIndex=" + CellUtil.toString(getNextIndexedKey(), false) + + ", before=" + qcode + ", after=" + after); + return after; + }; + + @Override + public Cell getNextIndexedKey() { + // Return the 2nd row and then the 3rd row dependent on how many times we've been through + // as though the next 'block' begins w/ second row according to the index and the third + // block with the third. + return count.get() > 8? CellUtil.createFirstOnRow(CELL_GRID[12]): + count.get() > 4? CellUtil.createFirstOnRow(CELL_GRID[8]): + CellUtil.createFirstOnRow(CELL_GRID[4]); + } + }; + try { + List results = new ArrayList(); + while (scanner.next(results)) { + continue; + } + Assert.assertEquals(4, results.size()); + Assert.assertEquals("First qcode is SEEK_NEXT_COL and second INCLUDE_AND_SEEK_NEXT_ROW", + 2, count.get()); + } finally { + scanner.close(); + } + } + + /** + * Ensure the optimize Scan method in StoreScanner does not get in the way of a Get doing minimum + * work... seeking to start of block and then SKIPPING until we find the wanted Cell. + * This 'simple' scenario mimics case of all Cells fitting inside a single HFileBlock. + * See HBASE-15392. This test is a little cryptic. Takes a bit of staring to figure what it up to. + * @throws IOException + */ + @Test + public void testOptimizeAndGet() throws IOException { + List scanners = Arrays.asList( + new KeyValueScanner[] {new KeyValueScanFixture(CellComparator.COMPARATOR, CELL_GRID)}); + // First test a Get of the middle Cell in the row R2. Every Get is a Scan. Middle row has a + // qualifier of R2. + Get get = new Get(R2); + get.addColumn(CF, R2); + Scan scan = new Scan(get); + // Count of how often optimize is called. + final AtomicInteger count = new AtomicInteger(0); + StoreScanner scanner = + new StoreScanner(scan, scanInfo, scanType, scan.getFamilyMap().get(CF), scanners) { + protected org.apache.hadoop.hbase.regionserver.ScanQueryMatcher.MatchCode + optimize(org.apache.hadoop.hbase.regionserver.ScanQueryMatcher.MatchCode qcode, Cell cell) { + count.incrementAndGet(); + return super.optimize(qcode, cell); + }; + }; + try { + List results = new ArrayList(); + // For a Get there should be no more next's after the first call. + Assert.assertEquals(false, scanner.next(results)); + // Should be one result only. + Assert.assertEquals(1, results.size()); + // And we should have gone through optimize twice only. + Assert.assertEquals("First qcode is SEEK_NEXT_COL and second INCLUDE_AND_SEEK_NEXT_ROW", + 2, count.get()); + } finally { + scanner.close(); + } + } + + /** + * Ensure that optimize does not cause the Get to do more seeking than required. Optimize + * (see HBASE-15392) was causing us to seek all Cells in a block when a Get Scan if the next block + * index/start key was a different row to the current one. A bug. We'd call next too often + * because we had to exhaust all Cells in the current row making us load the next block just to + * discard what we read there. This test is a little cryptic. Takes a bit of staring to figure + * what it up to. + * @throws IOException + */ + @Test + public void testOptimizeAndGetWithFakedNextBlockIndexStart() throws IOException { + List scanners = Arrays.asList( + new KeyValueScanner[] {new KeyValueScanFixture(CellComparator.COMPARATOR, CELL_GRID)}); + // First test a Get of second column in the row R2. Every Get is a Scan. Second column has a + // qualifier of R2. + Get get = new Get(R2); + get.addColumn(CF, R2); + Scan scan = new Scan(get); + // Count of how often optimize is called. + final AtomicInteger count = new AtomicInteger(0); + StoreScanner scanner = + new StoreScanner(scan, scanInfo, scanType, scan.getFamilyMap().get(CF), scanners) { + protected org.apache.hadoop.hbase.regionserver.ScanQueryMatcher.MatchCode + optimize(org.apache.hadoop.hbase.regionserver.ScanQueryMatcher.MatchCode qcode, Cell cell) { + count.incrementAndGet(); + return super.optimize(qcode, cell); + }; + + @Override + public Cell getNextIndexedKey() { + // Fake out StoreScanner that the next 'block' is the first Cell of the 'next' row. This + // brings on the bug seen in HBASE-15392 where instead of returning when we had all we + // needed for our Get, instead, we'd keep skipping all Cells on the current row till we + // ran into the next row. CELL_GRID[8] is first Cell of the new row R3 (Our Get is against + // row R2). + return CellUtil.createFirstOnRow(CELL_GRID[8]); + } + }; + try { + List results = new ArrayList(); + // For a Get there should be no more next's after the first call. + Assert.assertEquals(false, scanner.next(results)); + // Should be one result only. + Assert.assertEquals(1, results.size()); + // And we should have gone through optimize twice only. + Assert.assertEquals("First qcode is SEEK_NEXT_COL and second INCLUDE_AND_SEEK_NEXT_ROW", + 2, count.get()); + } finally { + scanner.close(); + } + } + @Test public void testScanTimeRange() throws IOException { String r1 = "R1";