diff --git a/CHANGES.txt b/CHANGES.txt index 7bacb4f87c9..1387e7e305b 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -4,6 +4,8 @@ Release 0.93.0 - Unreleased HBASE-4132 Extend the WALActionsListener API to accomodate log archival (dhruba borthakur) HBASE-4461 Expose getRowOrBefore via Thrift (jgray) + HBASE-4433 avoid extra next (potentially a seek) if done with column/row + (kannan via jgray) BUGS diff --git a/src/main/java/org/apache/hadoop/hbase/regionserver/ExplicitColumnTracker.java b/src/main/java/org/apache/hadoop/hbase/regionserver/ExplicitColumnTracker.java index 520f7784145..e1cfdb92953 100644 --- a/src/main/java/org/apache/hadoop/hbase/regionserver/ExplicitColumnTracker.java +++ b/src/main/java/org/apache/hadoop/hbase/regionserver/ExplicitColumnTracker.java @@ -50,6 +50,12 @@ public class ExplicitColumnTracker implements ColumnTracker { private final int maxVersions; private final int minVersions; + + /** + * Contains the list of columns that the ExplicitColumnTracker is tracking. + * Each ColumnCount instance also tracks how many versions of the requested + * column have been returned. + */ private final List columns; private final List columnsToReuse; private int index; @@ -127,13 +133,22 @@ public class ExplicitColumnTracker implements ColumnTracker { int count = this.column.increment(); if(count >= maxVersions || (count >= minVersions && isExpired(timestamp))) { // Done with versions for this column + // Note: because we are done with this column, and are removing + // it from columns, we don't do a ++this.index. The index stays + // the same but the columns have shifted within the array such + // that index now points to the next column we are interested in. this.columns.remove(this.index); + resetTS(); - if(this.columns.size() == this.index) { - // Will not hit any more columns in this storefile + if (this.columns.size() == this.index) { + // We have served all the requested columns. this.column = null; + return ScanQueryMatcher.MatchCode.INCLUDE_AND_SEEK_NEXT_ROW; } else { + // We are done with current column; advance to next column + // of interest. this.column = this.columns.get(this.index); + return ScanQueryMatcher.MatchCode.INCLUDE_AND_SEEK_NEXT_COL; } } else { setTS(timestamp); @@ -144,15 +159,18 @@ public class ExplicitColumnTracker implements ColumnTracker { resetTS(); if (ret > 0) { - // Specified column is smaller than the current, skip to next column. + // The current KV is smaller than the column the ExplicitColumnTracker + // is interested in, so seek to that column of interest. return ScanQueryMatcher.MatchCode.SEEK_NEXT_COL; } - // Specified column is bigger than current column - // Move down current column and check again - if(ret <= -1) { - if(++this.index >= this.columns.size()) { - // No more to match, do not include, done with storefile + // The current KV is bigger than the column the ExplicitColumnTracker + // is interested in. That means there is no more data for the column + // of interest. Advance the ExplicitColumnTracker state to next + // column of interest, and check again. + if (ret <= -1) { + if (++this.index >= this.columns.size()) { + // No more to match, do not include, done with this row. return ScanQueryMatcher.MatchCode.SEEK_NEXT_ROW; // done_row } // This is the recursive case. diff --git a/src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java b/src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java index a461e2143f0..68cdac59167 100644 --- a/src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java +++ b/src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java @@ -352,5 +352,15 @@ public class ScanQueryMatcher { * Seek to next key which is given as hint. */ SEEK_NEXT_USING_HINT, + + /** + * Include KeyValue and done with column, seek to next. + */ + INCLUDE_AND_SEEK_NEXT_COL, + + /** + * Include KeyValue and done with row, seek to next. + */ + INCLUDE_AND_SEEK_NEXT_ROW, } } diff --git a/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java b/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java index a456f5287c7..ac2348e1b9d 100644 --- a/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java +++ b/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java @@ -258,8 +258,23 @@ class StoreScanner implements KeyValueScanner, InternalScanner, ChangedReadersOb //DebugPrint.println("SS peek kv = " + kv + " with qcode = " + qcode); switch(qcode) { case INCLUDE: + case INCLUDE_AND_SEEK_NEXT_ROW: + case INCLUDE_AND_SEEK_NEXT_COL: + results.add(copyKv); - this.heap.next(); + + if (qcode == ScanQueryMatcher.MatchCode.INCLUDE_AND_SEEK_NEXT_ROW) { + if (!matcher.moreRowsMayExistAfter(kv)) { + outResult.addAll(results); + return false; + } + reseek(matcher.getKeyForNextRow(kv)); + } else if (qcode == ScanQueryMatcher.MatchCode.INCLUDE_AND_SEEK_NEXT_COL) { + reseek(matcher.getKeyForNextColumn(kv)); + } else { + this.heap.next(); + } + if (limit > 0 && (results.size() == limit)) { break LOOP; } diff --git a/src/test/java/org/apache/hadoop/hbase/regionserver/TestBlocksRead.java b/src/test/java/org/apache/hadoop/hbase/regionserver/TestBlocksRead.java index 83a6d5222be..019fddd1479 100644 --- a/src/test/java/org/apache/hadoop/hbase/regionserver/TestBlocksRead.java +++ b/src/test/java/org/apache/hadoop/hbase/regionserver/TestBlocksRead.java @@ -201,8 +201,8 @@ public class TestBlocksRead extends HBaseTestCase { putData(FAMILY, "row", "col7", 7); region.flushcache(); - // Expected block reads: 2 - kvs = getData(FAMILY, "row", "col1", 2); + // Expected block reads: 1 + kvs = getData(FAMILY, "row", "col1", 1); assertEquals(1, kvs.length); verifyData(kvs[0], "row", "col1", 1); @@ -218,8 +218,8 @@ public class TestBlocksRead extends HBaseTestCase { verifyData(kvs[0], "row", "col2", 2); verifyData(kvs[1], "row", "col3", 3); - // Expected block reads: 4 - kvs = getData(FAMILY, "row", Arrays.asList("col5"), 4); + // Expected block reads: 3 + kvs = getData(FAMILY, "row", Arrays.asList("col5"), 3); assertEquals(1, kvs.length); verifyData(kvs[0], "row", "col5", 5); } @@ -248,15 +248,16 @@ public class TestBlocksRead extends HBaseTestCase { putData(FAMILY, "row", "col2", 4); region.flushcache(); - // Baseline expected blocks read: 3 - kvs = getData(FAMILY, "row", Arrays.asList("col1"), 3); + // Baseline expected blocks read: 2 + kvs = getData(FAMILY, "row", Arrays.asList("col1"), 2); assertEquals(1, kvs.length); verifyData(kvs[0], "row", "col1", 3); - // Baseline expected blocks read: 5 + // Baseline expected blocks read: 6 // This increase is a minor glitch due to: HBASE-4466. Once that - // is fixed this will drop back. The extra access will be a cache hit. - kvs = getData(FAMILY, "row", Arrays.asList("col1", "col2"), 5); + // is fixed this will drop back. The extra access will be a cache + // hit. + kvs = getData(FAMILY, "row", Arrays.asList("col1", "col2"), 6); assertEquals(2, kvs.length); verifyData(kvs[0], "row", "col1", 3); verifyData(kvs[1], "row", "col2", 4); @@ -271,8 +272,8 @@ public class TestBlocksRead extends HBaseTestCase { verifyData(kvs[0], "row", "col3", 5); // Get a column from older file. - // Baseline expected blocks read: 4 - kvs = getData(FAMILY, "row", Arrays.asList("col1"), 4); + // Baseline expected blocks read: 3 + kvs = getData(FAMILY, "row", Arrays.asList("col1"), 3); assertEquals(1, kvs.length); verifyData(kvs[0], "row", "col1", 3); @@ -290,10 +291,12 @@ public class TestBlocksRead extends HBaseTestCase { kvs = getData(FAMILY, "row", Arrays.asList("col1", "col2", "col3"), 6); assertEquals(0, kvs.length); - // File 5: Delete with post data timestamp and insert some older - // date in new files. + // File 5: Delete deleteFamily(FAMILY, "row", 10); region.flushcache(); + + // File 6: some more puts, but with timestamps older than the + // previous delete. putData(FAMILY, "row", "col1", 7); putData(FAMILY, "row", "col2", 8); putData(FAMILY, "row", "col3", 9); @@ -303,14 +306,17 @@ public class TestBlocksRead extends HBaseTestCase { kvs = getData(FAMILY, "row", Arrays.asList("col1", "col2", "col3"), 10); assertEquals(0, kvs.length); - // File 6: Put back new data + // File 7: Put back new data putData(FAMILY, "row", "col1", 11); putData(FAMILY, "row", "col2", 12); putData(FAMILY, "row", "col3", 13); region.flushcache(); - // Baseline expected blocks read: 13 - kvs = getData(FAMILY, "row", Arrays.asList("col1", "col2", "col3"), 13); + // Baseline expected blocks read: 21 + // This increase is a minor glitch due to: HBASE-4466. Once that + // is fixed this will drop back. The extra access will be a cache + // hit. The test case only has 13 blocks altogther! + kvs = getData(FAMILY, "row", Arrays.asList("col1", "col2", "col3"), 21); assertEquals(3, kvs.length); verifyData(kvs[0], "row", "col1", 11); verifyData(kvs[1], "row", "col2", 12); diff --git a/src/test/java/org/apache/hadoop/hbase/regionserver/TestExplicitColumnTracker.java b/src/test/java/org/apache/hadoop/hbase/regionserver/TestExplicitColumnTracker.java index 49ab86dd74b..0533fcb6a4b 100644 --- a/src/test/java/org/apache/hadoop/hbase/regionserver/TestExplicitColumnTracker.java +++ b/src/test/java/org/apache/hadoop/hbase/regionserver/TestExplicitColumnTracker.java @@ -77,11 +77,11 @@ public class TestExplicitColumnTracker extends HBaseTestCase { columns.add(col2); columns.add(col4); List expected = new ArrayList(); - expected.add(ScanQueryMatcher.MatchCode.SEEK_NEXT_COL); - expected.add(ScanQueryMatcher.MatchCode.INCLUDE); - expected.add(ScanQueryMatcher.MatchCode.SEEK_NEXT_COL); - expected.add(ScanQueryMatcher.MatchCode.INCLUDE); - expected.add(ScanQueryMatcher.MatchCode.SEEK_NEXT_ROW); + expected.add(ScanQueryMatcher.MatchCode.SEEK_NEXT_COL); // col1 + expected.add(ScanQueryMatcher.MatchCode.INCLUDE_AND_SEEK_NEXT_COL); // col2 + expected.add(ScanQueryMatcher.MatchCode.SEEK_NEXT_COL); // col3 + expected.add(ScanQueryMatcher.MatchCode.INCLUDE_AND_SEEK_NEXT_ROW); // col4 + expected.add(ScanQueryMatcher.MatchCode.SEEK_NEXT_ROW); // col5 int maxVersions = 1; //Create "Scanner" @@ -111,16 +111,16 @@ public class TestExplicitColumnTracker extends HBaseTestCase { expected.add(ScanQueryMatcher.MatchCode.SEEK_NEXT_COL); expected.add(ScanQueryMatcher.MatchCode.SEEK_NEXT_COL); - expected.add(ScanQueryMatcher.MatchCode.INCLUDE); - expected.add(ScanQueryMatcher.MatchCode.INCLUDE); + expected.add(ScanQueryMatcher.MatchCode.INCLUDE); // col2; 1st version + expected.add(ScanQueryMatcher.MatchCode.INCLUDE_AND_SEEK_NEXT_COL); // col2; 2nd version expected.add(ScanQueryMatcher.MatchCode.SEEK_NEXT_COL); expected.add(ScanQueryMatcher.MatchCode.SEEK_NEXT_COL); expected.add(ScanQueryMatcher.MatchCode.SEEK_NEXT_COL); expected.add(ScanQueryMatcher.MatchCode.SEEK_NEXT_COL); - expected.add(ScanQueryMatcher.MatchCode.INCLUDE); - expected.add(ScanQueryMatcher.MatchCode.INCLUDE); + expected.add(ScanQueryMatcher.MatchCode.INCLUDE); // col4; 1st version + expected.add(ScanQueryMatcher.MatchCode.INCLUDE_AND_SEEK_NEXT_ROW); // col4; 2nd version expected.add(ScanQueryMatcher.MatchCode.SEEK_NEXT_ROW); expected.add(ScanQueryMatcher.MatchCode.SEEK_NEXT_ROW); diff --git a/src/test/java/org/apache/hadoop/hbase/regionserver/TestQueryMatcher.java b/src/test/java/org/apache/hadoop/hbase/regionserver/TestQueryMatcher.java index 83991752b94..bbe58f87ae3 100644 --- a/src/test/java/org/apache/hadoop/hbase/regionserver/TestQueryMatcher.java +++ b/src/test/java/org/apache/hadoop/hbase/regionserver/TestQueryMatcher.java @@ -88,10 +88,10 @@ public class TestQueryMatcher extends HBaseTestCase { //Expected result List expected = new ArrayList(); expected.add(ScanQueryMatcher.MatchCode.SEEK_NEXT_COL); - expected.add(ScanQueryMatcher.MatchCode.INCLUDE); + expected.add(ScanQueryMatcher.MatchCode.INCLUDE_AND_SEEK_NEXT_COL); expected.add(ScanQueryMatcher.MatchCode.SEEK_NEXT_COL); - expected.add(ScanQueryMatcher.MatchCode.INCLUDE); - expected.add(ScanQueryMatcher.MatchCode.INCLUDE); + expected.add(ScanQueryMatcher.MatchCode.INCLUDE_AND_SEEK_NEXT_COL); + expected.add(ScanQueryMatcher.MatchCode.INCLUDE_AND_SEEK_NEXT_ROW); expected.add(ScanQueryMatcher.MatchCode.DONE); // 2,4,5 @@ -182,9 +182,9 @@ public class TestQueryMatcher extends HBaseTestCase { long testTTL = 1000; MatchCode [] expected = new MatchCode[] { ScanQueryMatcher.MatchCode.SEEK_NEXT_COL, - ScanQueryMatcher.MatchCode.INCLUDE, + ScanQueryMatcher.MatchCode.INCLUDE_AND_SEEK_NEXT_COL, ScanQueryMatcher.MatchCode.SEEK_NEXT_COL, - ScanQueryMatcher.MatchCode.INCLUDE, + ScanQueryMatcher.MatchCode.INCLUDE_AND_SEEK_NEXT_COL, ScanQueryMatcher.MatchCode.SEEK_NEXT_ROW, ScanQueryMatcher.MatchCode.DONE };