You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hbase.apache.org by op...@apache.org on 2019/05/31 07:15:19 UTC
[hbase] 09/17: HBASE-22211 Remove the returnBlock method because we
can just call HFileBlock#release directly
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 <op...@gmail.com>
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;
}