Index: hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheOnWrite.java =================================================================== --- hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheOnWrite.java (revision 1578982) +++ hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheOnWrite.java (working copy) @@ -247,7 +247,7 @@ HFileBlock block = reader.readBlock(offset, onDiskSize, false, true, false, null); BlockCacheKey blockCacheKey = new BlockCacheKey(reader.getName(), - offset, encodingInCache, block.getBlockType()); + offset); boolean isCached = blockCache.getBlock(blockCacheKey, true, false) != null; boolean shouldBeCached = cowType.shouldBeCached(block.getBlockType()); if (shouldBeCached != isCached) { Index: hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestLruBlockCache.java =================================================================== --- hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestLruBlockCache.java (revision 1578982) +++ hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestLruBlockCache.java (working copy) @@ -125,6 +125,15 @@ assertEquals(buf.heapSize(), block.heapSize()); } + // Re-add same blocks and ensure nothing has changed + long expectedBlockCount = cache.getBlockCount(); + for (CachedItem block : blocks) { + cache.cacheBlock(block.cacheKey, block); + } + assertEquals( + "Cache should ignore cache requests for blocks already in cache", + expectedBlockCount, cache.getBlockCount()); + // Verify correctly calculated cache heap size assertEquals(expectedCacheSize, cache.heapSize()); Index: hbase-server/src/test/java/org/apache/hadoop/hbase/io/TestHeapSize.java =================================================================== --- hbase-server/src/test/java/org/apache/hadoop/hbase/io/TestHeapSize.java (revision 1578982) +++ hbase-server/src/test/java/org/apache/hadoop/hbase/io/TestHeapSize.java (working copy) @@ -337,11 +337,10 @@ assertEquals(expected, actual); } - // Block cache key overhead + // Block cache key overhead. Only tests fixed overhead as estimating heap + // size of strings is hard. cl = BlockCacheKey.class; - // Passing zero length file name, because estimateBase does not handle - // deep overhead. - actual = new BlockCacheKey("", 0).heapSize(); + actual = BlockCacheKey.FIXED_OVERHEAD; expected = ClassSize.estimateBase(cl, false); if (expected != actual) { ClassSize.estimateBase(cl, true); Index: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java =================================================================== --- hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java (revision 1578982) +++ hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java (working copy) @@ -75,6 +75,13 @@ return encoding; } + public boolean useEncodedScanner(boolean isCompaction) { + if (isCompaction && encoding == DataBlockEncoding.NONE) { + return false; + } + return encoding != DataBlockEncoding.NONE; + } + /** * Precondition: a non-encoded buffer. Postcondition: on-disk encoding. * Index: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderV2.java =================================================================== --- hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderV2.java (revision 1578982) +++ hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderV2.java (working copy) @@ -209,6 +209,21 @@ return new ScannerV2(this, cacheBlocks, pread, isCompaction); } + private HFileBlock getCachedBlock(BlockCacheKey cacheKey, boolean cacheBlock, boolean useLock, + boolean isCompaction, BlockType expectedBlockType) throws IOException { + // Check cache for block. If found return. + if (cacheConf.isBlockCacheEnabled()) { + BlockCache cache = cacheConf.getBlockCache(); + HFileBlock cachedBlock = + (HFileBlock) cache.getBlock(cacheKey, cacheBlock, useLock); + if (cachedBlock != null) { + validateBlockType(cachedBlock, expectedBlockType); + + return cachedBlock; + } + } + return null; + } /** * @param metaBlockName * @param cacheBlock Add block to cache, if found @@ -239,13 +254,12 @@ synchronized (metaBlockIndexReader.getRootBlockKey(block)) { // Check cache for block. If found return. long metaBlockOffset = metaBlockIndexReader.getRootBlockOffset(block); - BlockCacheKey cacheKey = new BlockCacheKey(name, metaBlockOffset, - DataBlockEncoding.NONE, BlockType.META); + BlockCacheKey cacheKey = new BlockCacheKey(name, metaBlockOffset); cacheBlock &= cacheConf.shouldCacheDataOnRead(); if (cacheConf.isBlockCacheEnabled()) { - HFileBlock cachedBlock = - (HFileBlock) cacheConf.getBlockCache().getBlock(cacheKey, cacheBlock, false); + HFileBlock cachedBlock = getCachedBlock(cacheKey, cacheBlock, false, false, + BlockType.META); if (cachedBlock != null) { // Return a distinct 'shallow copy' of the block, // so pos does not get messed by the scanner @@ -271,7 +285,8 @@ } /** - * Read in a file block. + * Read in a file block of the given {@link BlockType} and + * {@link DataBlockEncoding}. * @param dataBlockOffset offset to read. * @param onDiskBlockSize size of the block * @param cacheBlock @@ -306,9 +321,7 @@ // from doing). BlockCacheKey cacheKey = - new BlockCacheKey(name, dataBlockOffset, - dataBlockEncoder.getDataBlockEncoding(), - expectedBlockType); + new BlockCacheKey(name, dataBlockOffset); boolean useLock = false; IdLock.Entry lockEntry = null; @@ -323,8 +336,8 @@ if (cacheConf.isBlockCacheEnabled()) { // Try and get the block from the block cache. If the useLock variable is true then this // is the second time through the loop and it should not be counted as a block cache miss. - HFileBlock cachedBlock = (HFileBlock) cacheConf.getBlockCache().getBlock(cacheKey, - cacheBlock, useLock); + HFileBlock cachedBlock = getCachedBlock(cacheKey, cacheBlock, useLock, isCompaction, + expectedBlockType); if (cachedBlock != null) { validateBlockType(cachedBlock, expectedBlockType); if (cachedBlock.getBlockType().isData()) { @@ -398,8 +411,7 @@ return; } BlockType actualBlockType = block.getBlockType(); - if (actualBlockType == BlockType.ENCODED_DATA && - expectedBlockType == BlockType.DATA) { + if (expectedBlockType.isData() && actualBlockType.isData()) { // We consider DATA to match ENCODED_DATA for the purpose of this // verification. return; @@ -610,6 +622,7 @@ return curBlock; } + /** * Compare the given key against the current key * @param comparator Index: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java =================================================================== --- hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java (revision 1578982) +++ hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java (working copy) @@ -438,6 +438,19 @@ /** An abstraction used by the block index */ public interface CachingBlockReader { + /** + * Read in a file block. + * @param offset offset to read. + * @param onDiskBlockSize size of the block + * @param cacheBlock + * @param pread + * @param isCompaction is this block being read as part of a compaction + * @param expectedBlockType the block type we are expecting to read with this read operation, or + * null to read whatever block type is available and avoid checking (that might reduce + * caching efficiency of encoded data blocks) + * @return Block wrapped in a ByteBuffer. + * @throws IOException + */ HFileBlock readBlock(long offset, long onDiskBlockSize, boolean cacheBlock, final boolean pread, final boolean isCompaction, BlockType expectedBlockType) Index: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/BlockCacheKey.java =================================================================== --- hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/BlockCacheKey.java (revision 1578982) +++ hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/BlockCacheKey.java (working copy) @@ -19,7 +19,6 @@ import org.apache.hadoop.classification.InterfaceAudience; import org.apache.hadoop.hbase.io.HeapSize; -import org.apache.hadoop.hbase.io.encoding.DataBlockEncoding; import org.apache.hadoop.hbase.util.Bytes; import org.apache.hadoop.hbase.util.ClassSize; @@ -31,39 +30,27 @@ private static final long serialVersionUID = -5199992013113130534L; private final String hfileName; private final long offset; - private final DataBlockEncoding encoding; - public BlockCacheKey(String file, long offset, DataBlockEncoding encoding, - BlockType blockType) { - this.hfileName = file; - this.offset = offset; - // We add encoding to the cache key only for data blocks. If the block type - // is unknown (this should never be the case in production), we just use - // the provided encoding, because it might be a data block. - this.encoding = (encoding != null && (blockType == null - || blockType.isData())) ? encoding : DataBlockEncoding.NONE; - } - /** * Construct a new BlockCacheKey - * @param file The name of the HFile this block belongs to. + * @param hfileName The name of the HFile this block belongs to. * @param offset Offset of the block into the file */ - public BlockCacheKey(String file, long offset) { - this(file, offset, DataBlockEncoding.NONE, null); + public BlockCacheKey(String hfileName, long offset) { + this.hfileName = hfileName; + this.offset = offset; } @Override public int hashCode() { - return hfileName.hashCode() * 127 + (int) (offset ^ (offset >>> 32)) + - encoding.ordinal() * 17; + return hfileName.hashCode() * 127 + (int) (offset ^ (offset >>> 32)); } @Override public boolean equals(Object o) { if (o instanceof BlockCacheKey) { BlockCacheKey k = (BlockCacheKey) o; - return offset == k.offset && encoding == k.encoding + return offset == k.offset && (hfileName == null ? k.hfileName == null : hfileName .equals(k.hfileName)); } else { @@ -73,18 +60,21 @@ @Override public String toString() { - return hfileName + "_" + offset - + (encoding == DataBlockEncoding.NONE ? "" : "_" + encoding); + return String.format("%s_%d", hfileName, offset); } + public static final long FIXED_OVERHEAD = ClassSize.align(ClassSize.OBJECT + + ClassSize.REFERENCE + // this.hfileName + Bytes.SIZEOF_LONG); // this.offset + /** * Strings have two bytes per character due to default Java Unicode encoding * (hence length times 2). */ @Override public long heapSize() { - return ClassSize.align(ClassSize.OBJECT + 2 * hfileName.length() + - Bytes.SIZEOF_LONG + 2 * ClassSize.REFERENCE); + return ClassSize.align(FIXED_OVERHEAD + ClassSize.STRING + + 2 * hfileName.length()); } // can't avoid this unfortunately @@ -95,10 +85,6 @@ return hfileName; } - public DataBlockEncoding getDataBlockEncoding() { - return encoding; - } - public long getOffset() { return offset; } Index: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterV2.java =================================================================== --- hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterV2.java (revision 1578982) +++ hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterV2.java (working copy) @@ -196,8 +196,7 @@ private void doCacheOnWrite(long offset) { HFileBlock cacheFormatBlock = fsBlockWriter.getBlockForCaching(); cacheConf.getBlockCache().cacheBlock( - new BlockCacheKey(name, offset, blockEncoder.getDataBlockEncoding(), - cacheFormatBlock.getBlockType()), cacheFormatBlock); + new BlockCacheKey(name, offset), cacheFormatBlock); } /** Index: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java =================================================================== --- hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java (revision 1578982) +++ hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java (working copy) @@ -347,10 +347,6 @@ /** * Cache the block with the specified name and buffer. *

- * It is assumed this will NEVER be called on an already cached block. If - * that is done, it is assumed that you are reinserting the same exact - * block due to a race condition and will update the buffer but not modify - * the size of the cache. * @param cacheKey block's cache key * @param buf block buffer */ @@ -381,7 +377,7 @@ * @param repeat Whether this is a repeat lookup for the same block * (used to avoid double counting cache misses when doing double-check locking) * @return buffer of specified cache key, or null if not in cache - * @see HFileReaderV2#readBlock(long, long, boolean, boolean, boolean, BlockType) + * @see HFileReaderV2#readBlock(long, long, boolean, boolean, boolean, BlockType, DataBlockEncoding) */ @Override public Cacheable getBlock(BlockCacheKey cacheKey, boolean caching, boolean repeat) { @@ -917,8 +913,9 @@ public Map getEncodingCountsForTest() { Map counts = new EnumMap(DataBlockEncoding.class); - for (BlockCacheKey cacheKey : map.keySet()) { - DataBlockEncoding encoding = cacheKey.getDataBlockEncoding(); + for (CachedBlock block : map.values()) { + DataBlockEncoding encoding = + ((HFileBlock) block.getBuffer()).getDataBlockEncoding(); Integer count = counts.get(encoding); counts.put(encoding, (count == null ? 0 : count) + 1); } Index: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlockIndex.java =================================================================== --- hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlockIndex.java (revision 1578982) +++ hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlockIndex.java (working copy) @@ -167,8 +167,10 @@ * @param key the key we are looking for * @param keyOffset the offset of the key in its byte array * @param keyLength the length of the key - * @param currentBlock the current block, to avoid re-reading the same - * block + * @param currentBlock the current block, to avoid re-reading the same block + * @param cacheBlocks + * @param pread + * @param isCompaction * @return reader a basic way to load blocks * @throws IOException */ @@ -950,8 +952,7 @@ if (blockCache != null) { HFileBlock blockForCaching = blockWriter.getBlockForCaching(); blockCache.cacheBlock(new BlockCacheKey(nameForCaching, - beginOffset, DataBlockEncoding.NONE, - blockForCaching.getBlockType()), blockForCaching); + beginOffset), blockForCaching); } // Add intermediate index block size