You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hbase.apache.org by mb...@apache.org on 2012/08/28 23:14:48 UTC
svn commit: r1378349 - in /hbase/branches/0.89-fb/src:
main/java/org/apache/hadoop/hbase/io/hfile/HFile.java
main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderV2.java
test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java
Author: mbautin
Date: Tue Aug 28 21:14:48 2012
New Revision: 1378349
URL: http://svn.apache.org/viewvc?rev=1378349&view=rev
Log:
[HBASE-5898] Consider double-checked locking for block cache lock
Author: liyintang
Summary:
I understand the intension of IdLock is to avoid duplicatedly loading data to cache.
However, I believe it is more reasonable to acquire this IdLock ONLY when the cache misses (assuming the cache is enabled).
The sequence may be:
1) check block has been cached or not
2) If cached, just return the cached data.
3) If not, acquire the IdLock
4) Acquired the IdLock and check the block cache again
5) If cache misses, load the data into cache and return the data
6) If cache hits, return the cached data.
In this way, we don't need to pay this lock overhead if this particular block has already been cached.
Test Plan: tested on the ODS shadow and production cluster for a while
Reviewers: kannan, kranganathan, michalgr
Reviewed By: kannan
CC: hbase-eng@, sdong
Differential Revision: https://phabricator.fb.com/D552051
Modified:
hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java
hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderV2.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/HFile.java
URL: http://svn.apache.org/viewvc/hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java?rev=1378349&r1=1378348&r2=1378349&view=diff
==============================================================================
--- hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java (original)
+++ hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java Tue Aug 28 21:14:48 2012
@@ -170,9 +170,6 @@ public class HFile {
static final AtomicInteger preadOps = new AtomicInteger();
static final AtomicLong preadTimeNano = new AtomicLong();
- // for test purpose
- public static volatile AtomicLong dataBlockReadCnt = new AtomicLong(0);
-
// number of sequential reads
public static final int getReadOps() {
return readOps.getAndSet(0);
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=1378349&r1=1378348&r2=1378349&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 Tue Aug 28 21:14:48 2012
@@ -277,41 +277,21 @@ public class HFileReaderV2 extends Abstr
new BlockCacheKey(name, dataBlockOffset,
dataBlockEncoder.getEffectiveEncodingInCache(isCompaction),
expectedBlockType);
+
+ // Checking the block cache.
+ HFileBlock cachedBlock = this.getCachedBlock(cacheKey, cacheBlock, isCompaction,
+ expectedBlockType, true);
+ if (cachedBlock != null) {
+ return cachedBlock;
+ }
+
IdLock.Entry lockEntry = offsetLock.getLockEntry(dataBlockOffset);
try {
- // Check cache for block. If found return.
- if (cacheConf.isBlockCacheEnabled()) {
- HFileBlock cachedBlock = (HFileBlock)
- cacheConf.getBlockCache().getBlock(cacheKey, cacheBlock);
- if (cachedBlock != null) {
- BlockCategory blockCategory =
- cachedBlock.getBlockType().getCategory();
- getSchemaMetrics().updateOnCacheHit(blockCategory, isCompaction);
-
- if (cachedBlock.getBlockType() == BlockType.DATA) {
- HFile.dataBlockReadCnt.incrementAndGet();
- }
-
- validateBlockType(cachedBlock, expectedBlockType);
-
- // Validate encoding type for encoded blocks. We include encoding
- // type in the cache key, and we expect it to match on a cache hit.
- if (cachedBlock.getBlockType() == BlockType.ENCODED_DATA &&
- cachedBlock.getDataBlockEncoding() !=
- dataBlockEncoder.getEncodingInCache()) {
- throw new IOException("Cached block under key " + cacheKey + " " +
- "has wrong encoding: " + cachedBlock.getDataBlockEncoding() +
- " (expected: " + dataBlockEncoder.getEncodingInCache() + ")");
- }
- ProfilingData pData = HRegionServer.threadLocalProfilingData.get();
- if (pData != null) {
- pData.incInt(ProfilingData.blockHitStr(
- cachedBlock.getBlockType().getCategory(),
- cachedBlock.getColumnFamilyName()));
- }
- return cachedBlock;
- }
- // Carry on, please load.
+ // Double checking the block cache again within the IdLock
+ cachedBlock = this.getCachedBlock(cacheKey, cacheBlock, isCompaction,
+ expectedBlockType, false);
+ if (cachedBlock != null) {
+ return cachedBlock;
}
// Load block from filesystem.
@@ -353,6 +333,44 @@ public class HFileReaderV2 extends Abstr
}
}
+ private HFileBlock getCachedBlock(BlockCacheKey cacheKey, boolean cacheBlock,
+ boolean isCompaction, BlockType expectedBlockType, boolean updateMetrics) throws IOException {
+ // Check cache for block. If found return.
+ if (cacheConf.isBlockCacheEnabled()) {
+ HFileBlock cachedBlock = (HFileBlock)
+ cacheConf.getBlockCache().getBlock(cacheKey, cacheBlock);
+ if (cachedBlock != null) {
+ // Validate the block type first
+ validateBlockType(cachedBlock, expectedBlockType);
+
+ // Validate encoding type for encoded blocks. We include encoding
+ // type in the cache key, and we expect it to match on a cache hit.
+ if (cachedBlock.getBlockType() == BlockType.ENCODED_DATA &&
+ cachedBlock.getDataBlockEncoding() !=
+ dataBlockEncoder.getEncodingInCache()) {
+ throw new IOException("Cached block under key " + cacheKey + " " +
+ "has wrong encoding: " + cachedBlock.getDataBlockEncoding() +
+ " (expected: " + dataBlockEncoder.getEncodingInCache() + ")");
+ }
+
+ // Update the metrics if enabled
+ if (updateMetrics) {
+ BlockCategory blockCategory =
+ cachedBlock.getBlockType().getCategory();
+ getSchemaMetrics().updateOnCacheHit(blockCategory, isCompaction);
+
+ ProfilingData pData = HRegionServer.threadLocalProfilingData.get();
+ if (pData != null) {
+ pData.incInt(ProfilingData.blockHitStr(
+ cachedBlock.getBlockType().getCategory(),
+ cachedBlock.getColumnFamilyName()));
+ }
+ }
+ return cachedBlock;
+ }
+ }
+ return null;
+ }
/**
* Compares the actual type of a block retrieved from cache or disk with its
* expected type and throws an exception in case of a mismatch. Expected
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=1378349&r1=1378348&r2=1378349&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 Tue Aug 28 21:14:48 2012
@@ -761,9 +761,10 @@ public class TestStoreFile extends HBase
scanner.seek(KeyValue.LOWESTKEY);
while (scanner.next() != null);
assertEquals(startHit, cs.getHitCount());
- assertEquals(startMiss + 3, cs.getMissCount());
+ // since [HBASE-5898], region server will query the block cache twice in the cache miss cases.
+ assertEquals(startMiss + (2 * 3), cs.getMissCount());
assertEquals(startEvicted, cs.getEvictedCount());
- startMiss += 3;
+ startMiss += 2 * 3;
scanner.close();
reader.close(cacheConf.shouldEvictOnClose());
CacheTestHelper.forceDelayedEviction(bc);