This is an automated email from the ASF dual-hosted git repository. openinx pushed a commit to branch HBASE-21879 in repository https://gitbox.apache.org/repos/asf/hbase.git
commit d4e41fde8b9f77d5603db1a6e9f02339d52a10df Author: huzheng <[email protected]> AuthorDate: Mon Apr 22 15:57:12 2019 +0800 HBASE-22211 Remove the returnBlock method because we can just call HFileBlock#release directly --- .../apache/hadoop/hbase/io/hfile/BlockCache.java | 24 ----------- .../hadoop/hbase/io/hfile/BlockCacheUtil.java | 4 +- .../hadoop/hbase/io/hfile/CombinedBlockCache.java | 6 --- .../hadoop/hbase/io/hfile/CompoundBloomFilter.java | 10 ++--- .../org/apache/hadoop/hbase/io/hfile/HFile.java | 6 --- .../hadoop/hbase/io/hfile/HFileBlockIndex.java | 17 ++++---- .../hadoop/hbase/io/hfile/HFileReaderImpl.java | 46 ++++++---------------- .../hadoop/hbase/io/hfile/LruBlockCache.java | 4 +- .../hadoop/hbase/regionserver/StoreFileReader.java | 6 ++- .../hadoop/hbase/io/hfile/TestCacheOnWrite.java | 6 +-- .../apache/hadoop/hbase/io/hfile/TestHFile.java | 2 +- .../hadoop/hbase/io/hfile/TestHFileBlockIndex.java | 4 -- .../hbase/regionserver/TestHeapMemoryManager.java | 4 -- 13 files changed, 37 insertions(+), 102 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/BlockCache.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/BlockCache.java index 570519c..6849a97 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/BlockCache.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/BlockCache.java @@ -21,7 +21,6 @@ package org.apache.hadoop.hbase.io.hfile; import java.util.Iterator; import org.apache.yetus.audience.InterfaceAudience; -import org.apache.hadoop.hbase.io.hfile.Cacheable.MemoryType; /** * Block cache interface. Anything that implements the {@link Cacheable} @@ -133,27 +132,4 @@ public interface BlockCache extends Iterable<CachedBlock> { * @return The list of sub blockcaches that make up this one; returns null if no sub caches. */ BlockCache [] getBlockCaches(); - - /** - * Called when the scanner using the block decides to decrease refCnt of block and return the - * block once its usage is over. This API should be called after the block is used, failing to do - * so may have adverse effects by preventing the blocks from being evicted because of which it - * will prevent new hot blocks from getting added to the block cache. The implementation of the - * BlockCache will decide on what to be done with the block based on the memory type of the - * block's {@link MemoryType}. <br> - * <br> - * Note that if two handlers read from backingMap in off-heap BucketCache at the same time, BC - * will return two ByteBuff, which reference to the same memory area in buckets, but wrapped by - * two different ByteBuff, and each of them has its own independent refCnt(=1). so here, if - * returnBlock with different blocks in two handlers, it has no problem. but if both the two - * handlers returnBlock with the same block, then the refCnt exception will happen here. <br> - * TODO let's unify the ByteBuff's refCnt and BucketEntry's refCnt in HBASE-21957, after that - * we'll just call the Cacheable#release instead of calling release in some path and calling - * returnBlock in other paths in current version. - * @param cacheKey the cache key of the block - * @param block the hfileblock to be returned - */ - default void returnBlock(BlockCacheKey cacheKey, Cacheable block) { - block.release(); - } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/BlockCacheUtil.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/BlockCacheUtil.java index bf3a279..46e8e24 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/BlockCacheUtil.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/BlockCacheUtil.java @@ -247,8 +247,8 @@ public class BlockCacheUtil { return false; } } finally { - // return the block since we need to decrement the count - blockCache.returnBlock(cacheKey, existingBlock); + // Release this block to decrement the reference count. + existingBlock.release(); } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java index cb01540..36916359 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java @@ -379,12 +379,6 @@ public class CombinedBlockCache implements ResizableBlockCache, HeapSize { this.l1Cache.setMaxSize(size); } - @Override - public void returnBlock(BlockCacheKey cacheKey, Cacheable block) { - // returnBlock is meaningful for L2 cache alone. - this.l2Cache.returnBlock(cacheKey, block); - } - @VisibleForTesting public int getRpcRefCount(BlockCacheKey cacheKey) { return (this.l2Cache instanceof BucketCache) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CompoundBloomFilter.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CompoundBloomFilter.java index 2aceed7..29f29e1 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CompoundBloomFilter.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CompoundBloomFilter.java @@ -105,8 +105,8 @@ public class CompoundBloomFilter extends CompoundBloomFilterBase result = BloomFilterUtil.contains(key, keyOffset, keyLength, bloomBuf, bloomBlock.headerSize(), bloomBlock.getUncompressedSizeWithoutHeader(), hash, hashCount); } finally { - // After the use return back the block if it was served from a cache. - reader.returnBlock(bloomBlock); + // After the use, should release the block to deallocate byte buffers. + bloomBlock.release(); } if (numPositivesPerChunk != null && result) { // Update statistics. Only used in unit tests. @@ -144,10 +144,10 @@ public class CompoundBloomFilter extends CompoundBloomFilterBase try { ByteBuff bloomBuf = bloomBlock.getBufferReadOnly(); result = BloomFilterUtil.contains(keyCell, bloomBuf, bloomBlock.headerSize(), - bloomBlock.getUncompressedSizeWithoutHeader(), hash, hashCount, type); + bloomBlock.getUncompressedSizeWithoutHeader(), hash, hashCount, type); } finally { - // After the use return back the block if it was served from a cache. - reader.returnBlock(bloomBlock); + // After the use, should release the block to deallocate the byte buffers. + bloomBlock.release(); } if (numPositivesPerChunk != null && result) { // Update statistics. Only used in unit tests. diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java index 78ebedc..33e815e 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java @@ -407,12 +407,6 @@ public class HFile { final boolean updateCacheMetrics, BlockType expectedBlockType, DataBlockEncoding expectedDataBlockEncoding) throws IOException; - - /** - * Return the given block back to the cache, if it was obtained from cache. - * @param block Block to be returned. - */ - void returnBlock(HFileBlock block); } /** An interface used by clients to open and iterate an {@link HFile}. */ diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlockIndex.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlockIndex.java index 90d11ac..ad61839 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlockIndex.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlockIndex.java @@ -381,10 +381,9 @@ public class HFileBlockIndex { nextIndexedKey = tmpNextIndexKV; } } finally { - if (!dataBlock) { - // Return the block immediately if it is not the - // data block - cachingBlockReader.returnBlock(block); + if (!dataBlock && block != null) { + // Release the block immediately if it is not the data block + block.release(); } } } @@ -394,9 +393,11 @@ public class HFileBlockIndex { // Though we have retrieved a data block we have found an issue // in the retrieved data block. Hence returned the block so that // the ref count can be decremented - cachingBlockReader.returnBlock(block); - throw new IOException("Reached a data block at level " + lookupLevel + - " but the number of levels is " + searchTreeLevel); + if (block != null) { + block.release(); + } + throw new IOException("Reached a data block at level " + lookupLevel + + " but the number of levels is " + searchTreeLevel); } // set the next indexed key for the current block. @@ -436,7 +437,7 @@ public class HFileBlockIndex { byte[] bytes = b.toBytes(keyOffset, keyLen); targetMidKey = new KeyValue.KeyOnlyKeyValue(bytes, 0, bytes.length); } finally { - cachingBlockReader.returnBlock(midLeafBlock); + midLeafBlock.release(); } } else { // The middle of the root-level index. diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderImpl.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderImpl.java index 1137961..02e56e9 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderImpl.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderImpl.java @@ -291,7 +291,7 @@ public class HFileReaderImpl implements HFile.Reader, Configurable { // Ideally here the readBlock won't find the block in cache. We call this // readBlock so that block data is read from FS and cached in BC. we must call // returnBlock here to decrease the reference count of block. - returnBlock(block); + block.release(); } } } catch (IOException e) { @@ -377,20 +377,6 @@ public class HFileReaderImpl implements HFile.Reader, Configurable { return fileSize; } - @Override - public void returnBlock(HFileBlock block) { - if (block != null) { - if (this.cacheConf.getBlockCache().isPresent()) { - BlockCacheKey cacheKey = new BlockCacheKey(this.getFileContext().getHFileName(), - block.getOffset(), this.isPrimaryReplicaReader(), block.getBlockType()); - cacheConf.getBlockCache().get().returnBlock(cacheKey, block); - } else { - // Release the block here, it means the RPC path didn't ref to this block any more. - block.release(); - } - } - } - /** * @return the first key in the file. May be null if file has no entries. Note * that this is not the first row key, but rather the byte form of the @@ -553,23 +539,15 @@ public class HFileReaderImpl implements HFile.Reader, Configurable { this.curBlock = null; } - private void returnBlock(HFileBlock block) { - if (LOG.isTraceEnabled()) { - LOG.trace("Returning the block : " + block); - } - this.reader.returnBlock(block); - } - private void returnBlocks(boolean returnAll) { - for (int i = 0; i < this.prevBlocks.size(); i++) { - returnBlock(this.prevBlocks.get(i)); - } + this.prevBlocks.forEach(HFileBlock::release); this.prevBlocks.clear(); if (returnAll && this.curBlock != null) { - returnBlock(this.curBlock); + this.curBlock.release(); this.curBlock = null; } } + @Override public boolean isSeeked(){ return blockBuffer != null; @@ -897,7 +875,7 @@ public class HFileReaderImpl implements HFile.Reader, Configurable { // The first key in the current block 'seekToBlock' is greater than the given // seekBefore key. We will go ahead by reading the next block that satisfies the // given key. Return the current block before reading the next one. - reader.returnBlock(seekToBlock); + seekToBlock.release(); // It is important that we compute and pass onDiskSize to the block // reader so that it does not have to read the header separately to // figure out the size. Currently, we do not have a way to do this @@ -948,7 +926,7 @@ public class HFileReaderImpl implements HFile.Reader, Configurable { if (block != null && !block.getBlockType().isData()) { // Findbugs: NP_NULL_ON_SOME_PATH // Whatever block we read we will be returning it unless // it is a datablock. Just in case the blocks are non data blocks - reader.returnBlock(block); + block.release(); } } while (!block.getBlockType().isData()); @@ -1325,9 +1303,9 @@ public class HFileReaderImpl implements HFile.Reader, Configurable { if (cacheConf.shouldCacheCompressed(cachedBlock.getBlockType().getCategory())) { HFileBlock compressedBlock = cachedBlock; cachedBlock = compressedBlock.unpack(hfileContext, fsBlockReader); - // In case of compressed block after unpacking we can return the compressed block + // In case of compressed block after unpacking we can release the compressed block if (compressedBlock != cachedBlock) { - cache.returnBlock(cacheKey, compressedBlock); + compressedBlock.release(); } } validateBlockType(cachedBlock, expectedBlockType); @@ -1361,11 +1339,9 @@ public class HFileReaderImpl implements HFile.Reader, Configurable { // schema definition change. LOG.info("Evicting cached block with key " + cacheKey + " because of a data block encoding mismatch" + "; expected: " - + expectedDataBlockEncoding + ", actual: " + actualDataBlockEncoding + ", path=" - + path); - // This is an error scenario. so here we need to decrement the - // count. - cache.returnBlock(cacheKey, cachedBlock); + + expectedDataBlockEncoding + ", actual: " + actualDataBlockEncoding + ", path=" + path); + // This is an error scenario. so here we need to release the block. + cachedBlock.release(); cache.evictBlock(cacheKey); } return null; diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java index b01d014..82e64e7 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java @@ -530,8 +530,8 @@ public class LruBlockCache implements FirstLevelBlockCache { if (result instanceof HFileBlock && ((HFileBlock) result).usesSharedMemory()) { Cacheable original = result; result = ((HFileBlock) original).deepCloneOnHeap(); - // deepClone an new one, so need to put the original one back to free it. - victimHandler.returnBlock(cacheKey, original); + // deepClone an new one, so need to release the original one to deallocate it. + original.release(); } cacheBlock(cacheKey, result, /* inMemory = */ false); } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileReader.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileReader.java index e1fc918..0d4b6a6 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileReader.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileReader.java @@ -454,8 +454,10 @@ public class StoreFileReader { LOG.error("Bad bloom filter data -- proceeding without", e); setGeneralBloomFilterFaulty(); } finally { - // Return the bloom block so that its ref count can be decremented. - reader.returnBlock(bloomBlock); + // Release the bloom block so that its ref count can be decremented. + if (bloomBlock != null) { + bloomBlock.release(); + } } return true; } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheOnWrite.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheOnWrite.java index 60a4445..3a769b0 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheOnWrite.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheOnWrite.java @@ -336,15 +336,15 @@ public class TestCacheOnWrite { // Call return twice because for the isCache cased the counter would have got incremented // twice. Notice that here we need to returnBlock with different blocks. see comments in // BucketCache#returnBlock. - blockCache.returnBlock(blockCacheKey, blockPair.getSecond()); + blockPair.getSecond().release(); if (cacheCompressedData) { if (this.compress == Compression.Algorithm.NONE || cowType == CacheOnWriteType.INDEX_BLOCKS || cowType == CacheOnWriteType.BLOOM_BLOCKS) { - blockCache.returnBlock(blockCacheKey, blockPair.getFirst()); + blockPair.getFirst().release(); } } else { - blockCache.returnBlock(blockCacheKey, blockPair.getFirst()); + blockPair.getFirst().release(); } } } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFile.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFile.java index 0ed933b..101fd91 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFile.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFile.java @@ -223,7 +223,7 @@ public class TestHFile { Assert.assertTrue(hfb.isOnHeap()); } } finally { - combined.returnBlock(key, cachedBlock); + cachedBlock.release(); } block.release(); // return back the ByteBuffer back to allocator. } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlockIndex.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlockIndex.java index 6f8d0b0..faef386 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlockIndex.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlockIndex.java @@ -176,10 +176,6 @@ public class TestHFileBlockIndex { } @Override - public void returnBlock(HFileBlock block) { - } - - @Override public HFileBlock readBlock(long offset, long onDiskSize, boolean cacheBlock, boolean pread, boolean isCompaction, boolean updateCacheMetrics, BlockType expectedBlockType, diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHeapMemoryManager.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHeapMemoryManager.java index 8c9ce75..f8f73ca 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHeapMemoryManager.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHeapMemoryManager.java @@ -735,10 +735,6 @@ public class TestHeapMemoryManager { return null; } - @Override - public void returnBlock(BlockCacheKey cacheKey, Cacheable buf) { - } - public void setTestBlockSize(long testBlockSize) { this.testBlockSize = testBlockSize; }
