Author: mbautin Date: Thu Feb 2 22:53:49 2012 New Revision: 1239904 URL: http://svn.apache.org/viewvc?rev=1239904&view=rev Log: [jira] [HBASE-4589] [89-fb] CacheOnWrite broken in some cases because it can conflict with evictOnClose
Summary: Porting jgray's fix at https://issues.apache.org/jira/browse/HBASE-4589 to hbase-89-internal. We open and close a newly flushed StoreFile for verification, and the close operation happens to evict all cached-on-write blocks of the file. The fix adds a boolean parameter to the HFile close method. Submitting as an internal diff because HBASE-4542 is blocking updates to hbase-89-fb-apache at the moment, and CacheConfig is part of the delta, preventing this patch from applying. Test Plan: Unit tests, dev cluster Reviewers: liyintang, kannan, nspiegelberg Reviewed By: kannan CC: hbase-eng@lists Differential Revision: https://phabricator.fb.com/D399740 Revert Plan: OK Modified: hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderV2.java hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterV2.java hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java hbase/branches/0.89-fb/src/test/java/org/apache/hadoop/hbase/regionserver/HFileReadWriteTest.java hbase/branches/0.89-fb/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompoundBloomFilter.java hbase/branches/0.89-fb/src/test/java/org/apache/hadoop/hbase/regionserver/TestFSErrorsExposed.java hbase/branches/0.89-fb/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java Modified: hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderV2.java URL: http://svn.apache.org/viewvc/hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderV2.java?rev=1239904&r1=1239903&r2=1239904&view=diff ============================================================================== --- hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderV2.java (original) +++ hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderV2.java Thu Feb 2 22:53:49 2012 @@ -310,7 +310,7 @@ public class HFileReaderV2 extends Abstr @Override public void close(boolean evictOnClose) throws IOException { - if (cacheConf.shouldEvictOnClose()) { + if (evictOnClose && cacheConf.shouldEvictOnClose()) { int numEvicted = cacheConf.getBlockCache().evictBlocksByPrefix(name + HFile.CACHE_KEY_SEPARATOR); LOG.debug("On close of file " + name + " evicted " + numEvicted Modified: hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterV2.java URL: http://svn.apache.org/viewvc/hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterV2.java?rev=1239904&r1=1239903&r2=1239904&view=diff ============================================================================== --- hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterV2.java (original) +++ hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterV2.java Thu Feb 2 22:53:49 2012 @@ -27,6 +27,8 @@ import java.net.InetSocketAddress; import java.util.ArrayList; import java.util.List; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.FSDataOutputStream; import org.apache.hadoop.fs.FileSystem; @@ -44,6 +46,7 @@ import org.apache.hadoop.io.WritableUtil * Writes HFile format version 2. */ public class HFileWriterV2 extends AbstractHFileWriter { + static final Log LOG = LogFactory.getLog(HFileWriterV2.class); /** Max memstore (rwcc) timestamp in FileInfo */ public static final byte[] MAX_MEMSTORE_TS_KEY = @@ -211,6 +214,8 @@ public class HFileWriterV2 extends Abstr // Meta data block index writer metaBlockIndexWriter = new HFileBlockIndex.BlockIndexWriter(); + + LOG.debug("HFileWriter initialized with " + cacheConf); } /** Modified: hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java URL: http://svn.apache.org/viewvc/hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java?rev=1239904&r1=1239903&r2=1239904&view=diff ============================================================================== --- hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java (original) +++ hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java Thu Feb 2 22:53:49 2012 @@ -296,7 +296,7 @@ public class LoadIncrementalHFiles exten } } finally { if (halfWriter != null) halfWriter.close(); - if (halfReader != null) halfReader.close(); + if (halfReader != null) halfReader.close(cacheConf.shouldEvictOnClose()); } } Modified: hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java URL: http://svn.apache.org/viewvc/hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java?rev=1239904&r1=1239903&r2=1239904&view=diff ============================================================================== --- hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java (original) +++ hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java Thu Feb 2 22:53:49 2012 @@ -493,7 +493,7 @@ public class Store extends SchemaConfigu for (final StoreFile f : result) { completionService.submit(new Callable<Void>() { public Void call() throws IOException { - f.closeReader(); + f.closeReader(true); return null; } }); @@ -1357,7 +1357,7 @@ public class Store extends SchemaConfigu throw e; } finally { if (storeFile != null) { - storeFile.closeReader(); + storeFile.closeReader(false); } } } Modified: hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java URL: http://svn.apache.org/viewvc/hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java?rev=1239904&r1=1239903&r2=1239904&view=diff ============================================================================== --- hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java (original) +++ hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java Thu Feb 2 22:53:49 2012 @@ -492,12 +492,10 @@ public class StoreFile { return this.reader; } - /** - * @throws IOException - */ - public synchronized void closeReader() throws IOException { + public synchronized void closeReader(boolean evictOnClose) + throws IOException { if (this.reader != null) { - this.reader.close(); + this.reader.close(evictOnClose); this.reader = null; } } @@ -507,7 +505,7 @@ public class StoreFile { * @throws IOException */ public void deleteReader() throws IOException { - closeReader(); + closeReader(true); this.fs.delete(getPath(), true); } @@ -1136,6 +1134,10 @@ public class StoreFile { return reader.getScanner(cacheBlocks, pread, isCompaction); } + public void close(boolean evictOnClose) throws IOException { + reader.close(evictOnClose); + } + public void close() throws IOException { reader.close(); } Modified: hbase/branches/0.89-fb/src/test/java/org/apache/hadoop/hbase/regionserver/HFileReadWriteTest.java URL: http://svn.apache.org/viewvc/hbase/branches/0.89-fb/src/test/java/org/apache/hadoop/hbase/regionserver/HFileReadWriteTest.java?rev=1239904&r1=1239903&r2=1239904&view=diff ============================================================================== --- hbase/branches/0.89-fb/src/test/java/org/apache/hadoop/hbase/regionserver/HFileReadWriteTest.java (original) +++ hbase/branches/0.89-fb/src/test/java/org/apache/hadoop/hbase/regionserver/HFileReadWriteTest.java Thu Feb 2 22:53:49 2012 @@ -777,7 +777,7 @@ public class HFileReadWriteTest { } } finally { - storeFile.closeReader(); + storeFile.closeReader(true); exec.shutdown(); LruBlockCache c = (LruBlockCache) new CacheConfig(conf).getBlockCache(); Modified: hbase/branches/0.89-fb/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompoundBloomFilter.java URL: http://svn.apache.org/viewvc/hbase/branches/0.89-fb/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompoundBloomFilter.java?rev=1239904&r1=1239903&r2=1239904&view=diff ============================================================================== --- hbase/branches/0.89-fb/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompoundBloomFilter.java (original) +++ hbase/branches/0.89-fb/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompoundBloomFilter.java Thu Feb 2 22:53:49 2012 @@ -267,7 +267,7 @@ public class TestCompoundBloomFilter { } } - r.close(); + r.close(true); // end of test so evictOnClose } private boolean isInBloom(StoreFileScanner scanner, byte[] row, BloomType bt, Modified: hbase/branches/0.89-fb/src/test/java/org/apache/hadoop/hbase/regionserver/TestFSErrorsExposed.java URL: http://svn.apache.org/viewvc/hbase/branches/0.89-fb/src/test/java/org/apache/hadoop/hbase/regionserver/TestFSErrorsExposed.java?rev=1239904&r1=1239903&r2=1239904&view=diff ============================================================================== --- hbase/branches/0.89-fb/src/test/java/org/apache/hadoop/hbase/regionserver/TestFSErrorsExposed.java (original) +++ hbase/branches/0.89-fb/src/test/java/org/apache/hadoop/hbase/regionserver/TestFSErrorsExposed.java Thu Feb 2 22:53:49 2012 @@ -100,7 +100,7 @@ public class TestFSErrorsExposed { LOG.info("Got expected exception", ioe); assertTrue(ioe.getMessage().contains("Fault")); } - reader.close(); + reader.close(true); // end of test so evictOnClose } /** Modified: hbase/branches/0.89-fb/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java URL: http://svn.apache.org/viewvc/hbase/branches/0.89-fb/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java?rev=1239904&r1=1239903&r2=1239904&view=diff ============================================================================== --- hbase/branches/0.89-fb/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java (original) +++ hbase/branches/0.89-fb/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java Thu Feb 2 22:53:49 2012 @@ -326,10 +326,10 @@ public class TestStoreFile extends HBase assertTrue(count == 0); } finally { if (top != null) { - top.close(); + top.close(true); // evict since we are about to delete the file } if (bottom != null) { - bottom.close(); + bottom.close(true); // evict since we are about to delete the file } fs.delete(f.getPath(), true); } @@ -375,7 +375,7 @@ public class TestStoreFile extends HBase if (exists) falsePos++; } } - reader.close(); + reader.close(true); // evict because we are about to delete the file fs.delete(f, true); assertEquals("False negatives: " + falseNeg, 0, falseNeg); int maxFalsePos = (int) (2 * 2000 * err); @@ -525,7 +525,7 @@ public class TestStoreFile extends HBase } } } - reader.close(); + reader.close(true); // evict because we are about to delete the file fs.delete(f, true); System.out.println(bt[x].toString()); System.out.println(" False negatives: " + falseNeg); @@ -735,7 +735,7 @@ public class TestStoreFile extends HBase assertEquals(startEvicted, cs.getEvictedCount()); startMiss += 3; scanner.close(); - reader.close(); + reader.close(cacheConf.shouldEvictOnClose()); // Now write a StoreFile with three blocks, with cache on write on conf.setBoolean(CacheConfig.CACHE_BLOCKS_ON_WRITE_KEY, true); @@ -755,7 +755,7 @@ public class TestStoreFile extends HBase assertEquals(startEvicted, cs.getEvictedCount()); startHit += 3; scanner.close(); - reader.close(); + reader.close(cacheConf.shouldEvictOnClose()); // Let's read back the two files to ensure the blocks exactly match hsf = new StoreFile(this.fs, pathCowOff, conf, cacheConf, @@ -790,9 +790,9 @@ public class TestStoreFile extends HBase assertEquals(startEvicted, cs.getEvictedCount()); startHit += 6; scannerOne.close(); - readerOne.close(); + readerOne.close(cacheConf.shouldEvictOnClose()); scannerTwo.close(); - readerTwo.close(); + readerTwo.close(cacheConf.shouldEvictOnClose()); // Let's close the first file with evict on close turned on conf.setBoolean("hbase.rs.evictblocksonclose", true); @@ -800,7 +800,7 @@ public class TestStoreFile extends HBase hsf = new StoreFile(this.fs, pathCowOff, conf, cacheConf, StoreFile.BloomType.NONE); reader = hsf.createReader(); - reader.close(); + reader.close(cacheConf.shouldEvictOnClose()); // We should have 3 new evictions assertEquals(startHit, cs.getHitCount()); @@ -814,7 +814,7 @@ public class TestStoreFile extends HBase hsf = new StoreFile(this.fs, pathCowOn, conf, cacheConf, StoreFile.BloomType.NONE); reader = hsf.createReader(); - reader.close(); + reader.close(cacheConf.shouldEvictOnClose()); // We expect no changes assertEquals(startHit, cs.getHitCount());
