HBASE-16650 Wrong usage of BlockCache eviction stat for heap memory tuning.
Project: http://git-wip-us.apache.org/repos/asf/hbase/repo Commit: http://git-wip-us.apache.org/repos/asf/hbase/commit/1384c9a0 Tree: http://git-wip-us.apache.org/repos/asf/hbase/tree/1384c9a0 Diff: http://git-wip-us.apache.org/repos/asf/hbase/diff/1384c9a0 Branch: refs/heads/hbase-14439 Commit: 1384c9a08dd356556748ca0cc5853a695a40932c Parents: 4bb84f7 Author: anoopsamjohn <anoopsamj...@gmail.com> Authored: Thu Sep 22 21:28:30 2016 +0530 Committer: anoopsamjohn <anoopsamj...@gmail.com> Committed: Thu Sep 22 21:28:30 2016 +0530 ---------------------------------------------------------------------- .../regionserver/MetricsRegionServerSource.java | 3 +- .../hadoop/hbase/io/hfile/CacheConfig.java | 48 ++++++++++++++------ .../hadoop/hbase/io/hfile/LruBlockCache.java | 22 +++++---- .../hbase/regionserver/HeapMemoryManager.java | 8 ++-- .../hbase/io/hfile/TestBlockCacheReporting.java | 4 +- .../hadoop/hbase/io/hfile/TestCacheConfig.java | 8 ++-- .../io/hfile/TestForceCacheImportantBlocks.java | 2 +- .../hfile/TestLazyDataBlockDecompression.java | 4 +- .../io/hfile/TestScannerFromBucketCache.java | 2 +- .../hbase/regionserver/TestStoreFile.java | 6 +-- 10 files changed, 64 insertions(+), 43 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/hbase/blob/1384c9a0/hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionServerSource.java ---------------------------------------------------------------------- diff --git a/hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionServerSource.java b/hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionServerSource.java index a80745d..1e9b7ef 100644 --- a/hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionServerSource.java +++ b/hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionServerSource.java @@ -285,7 +285,8 @@ public interface MetricsRegionServerSource extends BaseSource, JvmPauseMonitorSo "Number of requests for a block of primary replica that missed the block cache."; String BLOCK_CACHE_EVICTION_COUNT = "blockCacheEvictionCount"; String BLOCK_CACHE_EVICTION_COUNT_DESC = - "Count of the number of blocks evicted from the block cache."; + "Count of the number of blocks evicted from the block cache." + + "(Not including blocks evicted because of HFile removal)"; String BLOCK_CACHE_PRIMARY_EVICTION_COUNT = "blockCacheEvictionCountPrimary"; String BLOCK_CACHE_PRIMARY_EVICTION_COUNT_DESC = "Count of the number of blocks evicted from primary replica in the block cache."; http://git-wip-us.apache.org/repos/asf/hbase/blob/1384c9a0/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CacheConfig.java ---------------------------------------------------------------------- diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CacheConfig.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CacheConfig.java index ffb9424..321f72c 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CacheConfig.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CacheConfig.java @@ -22,7 +22,6 @@ import static org.apache.hadoop.hbase.HConstants.BUCKET_CACHE_SIZE_KEY; import java.io.IOException; import java.lang.management.ManagementFactory; -import java.lang.management.MemoryUsage; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -532,12 +531,13 @@ public class CacheConfig { // Clear this if in tests you'd make more than one block cache instance. @VisibleForTesting static BlockCache GLOBAL_BLOCK_CACHE_INSTANCE; + private static LruBlockCache GLOBAL_L1_CACHE_INSTANCE; /** Boolean whether we have disabled the block cache entirely. */ @VisibleForTesting static boolean blockCacheDisabled = false; - static long getLruCacheSize(final Configuration conf, final MemoryUsage mu) { + static long getLruCacheSize(final Configuration conf, final long xmx) { float cachePercentage = conf.getFloat(HConstants.HFILE_BLOCK_CACHE_SIZE_KEY, HConstants.HFILE_BLOCK_CACHE_SIZE_DEFAULT); if (cachePercentage <= 0.0001f) { @@ -550,30 +550,41 @@ public class CacheConfig { } // Calculate the amount of heap to give the heap. - return (long) (mu.getMax() * cachePercentage); + return (long) (xmx * cachePercentage); } /** * @param c Configuration to use. - * @param mu JMX Memory Bean * @return An L1 instance. Currently an instance of LruBlockCache. */ - private static LruBlockCache getL1(final Configuration c, final MemoryUsage mu) { - long lruCacheSize = getLruCacheSize(c, mu); + public static LruBlockCache getL1(final Configuration c) { + return getL1(c, ManagementFactory.getMemoryMXBean().getHeapMemoryUsage().getMax()); + } + + /** + * @param c Configuration to use. + * @param xmx Max heap memory + * @return An L1 instance. Currently an instance of LruBlockCache. + */ + private synchronized static LruBlockCache getL1(final Configuration c, final long xmx) { + if (GLOBAL_L1_CACHE_INSTANCE != null) return GLOBAL_L1_CACHE_INSTANCE; + if (blockCacheDisabled) return null; + long lruCacheSize = getLruCacheSize(c, xmx); if (lruCacheSize < 0) return null; int blockSize = c.getInt(BLOCKCACHE_BLOCKSIZE_KEY, HConstants.DEFAULT_BLOCKSIZE); LOG.info("Allocating LruBlockCache size=" + StringUtils.byteDesc(lruCacheSize) + ", blockSize=" + StringUtils.byteDesc(blockSize)); - return new LruBlockCache(lruCacheSize, blockSize, true, c); + GLOBAL_L1_CACHE_INSTANCE = new LruBlockCache(lruCacheSize, blockSize, true, c); + return GLOBAL_L1_CACHE_INSTANCE; } /** * @param c Configuration to use. - * @param mu JMX Memory Bean + * @param xmx Max heap memory * @return Returns L2 block cache instance (for now it is BucketCache BlockCache all the time) * or null if not supposed to be a L2. */ - private static BlockCache getL2(final Configuration c, final MemoryUsage mu) { + private static BlockCache getL2(final Configuration c, final long xmx) { final boolean useExternal = c.getBoolean(EXTERNAL_BLOCKCACHE_KEY, EXTERNAL_BLOCKCACHE_DEFAULT); if (LOG.isDebugEnabled()) { LOG.debug("Trying to use " + (useExternal?" External":" Internal") + " l2 cache"); @@ -585,7 +596,7 @@ public class CacheConfig { } // otherwise use the bucket cache. - return getBucketCache(c, mu); + return getBucketCache(c, xmx); } @@ -615,14 +626,14 @@ public class CacheConfig { } - private static BlockCache getBucketCache(Configuration c, MemoryUsage mu) { + private static BlockCache getBucketCache(Configuration c, long xmx) { // Check for L2. ioengine name must be non-null. String bucketCacheIOEngineName = c.get(BUCKET_CACHE_IOENGINE_KEY, null); if (bucketCacheIOEngineName == null || bucketCacheIOEngineName.length() <= 0) return null; int blockSize = c.getInt(BLOCKCACHE_BLOCKSIZE_KEY, HConstants.DEFAULT_BLOCKSIZE); float bucketCachePercentage = c.getFloat(BUCKET_CACHE_SIZE_KEY, 0F); - long bucketCacheSize = (long) (bucketCachePercentage < 1? mu.getMax() * bucketCachePercentage: + long bucketCacheSize = (long) (bucketCachePercentage < 1? xmx * bucketCachePercentage: bucketCachePercentage * 1024 * 1024); if (bucketCacheSize <= 0) { throw new IllegalStateException("bucketCacheSize <= 0; Check " + @@ -670,11 +681,11 @@ public class CacheConfig { public static synchronized BlockCache instantiateBlockCache(Configuration conf) { if (GLOBAL_BLOCK_CACHE_INSTANCE != null) return GLOBAL_BLOCK_CACHE_INSTANCE; if (blockCacheDisabled) return null; - MemoryUsage mu = ManagementFactory.getMemoryMXBean().getHeapMemoryUsage(); - LruBlockCache l1 = getL1(conf, mu); + long xmx = ManagementFactory.getMemoryMXBean().getHeapMemoryUsage().getMax(); + LruBlockCache l1 = getL1(conf, xmx); // blockCacheDisabled is set as a side-effect of getL1(), so check it again after the call. if (blockCacheDisabled) return null; - BlockCache l2 = getL2(conf, mu); + BlockCache l2 = getL2(conf, xmx); if (l2 == null) { GLOBAL_BLOCK_CACHE_INSTANCE = l1; } else { @@ -698,4 +709,11 @@ public class CacheConfig { } return GLOBAL_BLOCK_CACHE_INSTANCE; } + + // Supposed to use only from tests. Some tests want to reinit the Global block cache instance + @VisibleForTesting + static synchronized void clearGlobalInstances() { + GLOBAL_L1_CACHE_INSTANCE = null; + GLOBAL_BLOCK_CACHE_INSTANCE = null; + } } http://git-wip-us.apache.org/repos/asf/hbase/blob/1384c9a0/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java ---------------------------------------------------------------------- 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 c56d7c6..99b67ba 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 @@ -550,15 +550,19 @@ public class LruBlockCache implements ResizableBlockCache, HeapSize { long size = map.size(); assertCounterSanity(size, val); } - stats.evicted(block.getCachedTime(), block.getCacheKey().isPrimary()); - if (evictedByEvictionProcess && victimHandler != null) { - if (victimHandler instanceof BucketCache) { - boolean wait = getCurrentSize() < acceptableSize(); - boolean inMemory = block.getPriority() == BlockPriority.MEMORY; - ((BucketCache)victimHandler).cacheBlockWithWait(block.getCacheKey(), block.getBuffer(), - inMemory, wait); - } else { - victimHandler.cacheBlock(block.getCacheKey(), block.getBuffer()); + if (evictedByEvictionProcess) { + // When the eviction of the block happened because of invalidation of HFiles, no need to + // update the stats counter. + stats.evicted(block.getCachedTime(), block.getCacheKey().isPrimary()); + if (victimHandler != null) { + if (victimHandler instanceof BucketCache) { + boolean wait = getCurrentSize() < acceptableSize(); + boolean inMemory = block.getPriority() == BlockPriority.MEMORY; + ((BucketCache) victimHandler).cacheBlockWithWait(block.getCacheKey(), block.getBuffer(), + inMemory, wait); + } else { + victimHandler.cacheBlock(block.getCacheKey(), block.getBuffer()); + } } } return block.heapSize(); http://git-wip-us.apache.org/repos/asf/hbase/blob/1384c9a0/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HeapMemoryManager.java ---------------------------------------------------------------------- diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HeapMemoryManager.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HeapMemoryManager.java index c360a60..6226cf2 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HeapMemoryManager.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HeapMemoryManager.java @@ -34,7 +34,6 @@ import org.apache.hadoop.hbase.HConstants; import org.apache.hadoop.hbase.ScheduledChore; import org.apache.hadoop.hbase.Server; import org.apache.hadoop.hbase.classification.InterfaceAudience; -import org.apache.hadoop.hbase.io.hfile.BlockCache; import org.apache.hadoop.hbase.io.hfile.CacheConfig; import org.apache.hadoop.hbase.io.hfile.ResizableBlockCache; import org.apache.hadoop.hbase.io.util.HeapMemorySizeUtil; @@ -93,10 +92,9 @@ public class HeapMemoryManager { public static HeapMemoryManager create(Configuration conf, FlushRequester memStoreFlusher, Server server, RegionServerAccounting regionServerAccounting) { - BlockCache blockCache = CacheConfig.instantiateBlockCache(conf); - if (blockCache instanceof ResizableBlockCache) { - return new HeapMemoryManager((ResizableBlockCache) blockCache, memStoreFlusher, server, - regionServerAccounting); + ResizableBlockCache l1Cache = CacheConfig.getL1(conf); + if (l1Cache != null) { + return new HeapMemoryManager(l1Cache, memStoreFlusher, server, regionServerAccounting); } return null; } http://git-wip-us.apache.org/repos/asf/hbase/blob/1384c9a0/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestBlockCacheReporting.java ---------------------------------------------------------------------- diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestBlockCacheReporting.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestBlockCacheReporting.java index 4080249..5651c13 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestBlockCacheReporting.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestBlockCacheReporting.java @@ -46,14 +46,14 @@ public class TestBlockCacheReporting { @Before public void setUp() throws Exception { - CacheConfig.GLOBAL_BLOCK_CACHE_INSTANCE = null; + CacheConfig.clearGlobalInstances(); this.conf = HBaseConfiguration.create(); } @After public void tearDown() throws Exception { // Let go of current block cache. - CacheConfig.GLOBAL_BLOCK_CACHE_INSTANCE = null; + CacheConfig.clearGlobalInstances(); } private void addDataAndHits(final BlockCache bc, final int count) { http://git-wip-us.apache.org/repos/asf/hbase/blob/1384c9a0/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheConfig.java ---------------------------------------------------------------------- diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheConfig.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheConfig.java index d8a2589..d9d5261 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheConfig.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheConfig.java @@ -159,14 +159,14 @@ public class TestCacheConfig { @Before public void setUp() throws Exception { - CacheConfig.GLOBAL_BLOCK_CACHE_INSTANCE = null; + CacheConfig.clearGlobalInstances(); this.conf = HBaseConfiguration.create(); } @After public void tearDown() throws Exception { // Let go of current block cache. - CacheConfig.GLOBAL_BLOCK_CACHE_INSTANCE = null; + CacheConfig.clearGlobalInstances(); } /** @@ -329,7 +329,7 @@ public class TestCacheConfig { assertTrue(bcs[0] instanceof LruBlockCache); LruBlockCache lbc = (LruBlockCache)bcs[0]; assertEquals(CacheConfig.getLruCacheSize(this.conf, - ManagementFactory.getMemoryMXBean().getHeapMemoryUsage()), lbc.getMaxSize()); + ManagementFactory.getMemoryMXBean().getHeapMemoryUsage().getMax()), lbc.getMaxSize()); assertTrue(bcs[1] instanceof BucketCache); BucketCache bc = (BucketCache)bcs[1]; // getMaxSize comes back in bytes but we specified size in MB @@ -347,7 +347,7 @@ public class TestCacheConfig { // from L1 happens, it does not fail because L2 can't take the eviction because block too big. this.conf.setFloat(HConstants.HFILE_BLOCK_CACHE_SIZE_KEY, 0.001f); MemoryUsage mu = ManagementFactory.getMemoryMXBean().getHeapMemoryUsage(); - long lruExpectedSize = CacheConfig.getLruCacheSize(this.conf, mu); + long lruExpectedSize = CacheConfig.getLruCacheSize(this.conf, mu.getMax()); final int bcSize = 100; long bcExpectedSize = 100 * 1024 * 1024; // MB. assertTrue(lruExpectedSize < bcExpectedSize); http://git-wip-us.apache.org/repos/asf/hbase/blob/1384c9a0/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestForceCacheImportantBlocks.java ---------------------------------------------------------------------- diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestForceCacheImportantBlocks.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestForceCacheImportantBlocks.java index 4c2217d..ece658b 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestForceCacheImportantBlocks.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestForceCacheImportantBlocks.java @@ -96,7 +96,7 @@ public class TestForceCacheImportantBlocks { @Before public void setup() { // Make sure we make a new one each time. - CacheConfig.GLOBAL_BLOCK_CACHE_INSTANCE = null; + CacheConfig.clearGlobalInstances(); HFile.DATABLOCK_READ_COUNT.reset(); } http://git-wip-us.apache.org/repos/asf/hbase/blob/1384c9a0/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestLazyDataBlockDecompression.java ---------------------------------------------------------------------- diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestLazyDataBlockDecompression.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestLazyDataBlockDecompression.java index 5f73500..cf3c6ed 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestLazyDataBlockDecompression.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestLazyDataBlockDecompression.java @@ -73,13 +73,13 @@ public class TestLazyDataBlockDecompression { @Before public void setUp() throws IOException { - CacheConfig.GLOBAL_BLOCK_CACHE_INSTANCE = null; + CacheConfig.clearGlobalInstances(); fs = FileSystem.get(TEST_UTIL.getConfiguration()); } @After public void tearDown() { - CacheConfig.GLOBAL_BLOCK_CACHE_INSTANCE = null; + CacheConfig.clearGlobalInstances(); fs = null; } http://git-wip-us.apache.org/repos/asf/hbase/blob/1384c9a0/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestScannerFromBucketCache.java ---------------------------------------------------------------------- diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestScannerFromBucketCache.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestScannerFromBucketCache.java index eb3dfa6..898f3bb 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestScannerFromBucketCache.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestScannerFromBucketCache.java @@ -91,7 +91,7 @@ public class TestScannerFromBucketCache { EnvironmentEdgeManagerTestHelper.reset(); LOG.info("Cleaning test directory: " + test_util.getDataTestDir()); test_util.cleanupTestDir(); - CacheConfig.GLOBAL_BLOCK_CACHE_INSTANCE = null; + CacheConfig.clearGlobalInstances(); } String getName() { http://git-wip-us.apache.org/repos/asf/hbase/blob/1384c9a0/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java ---------------------------------------------------------------------- diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java index f03533e..0a8dbc4 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java @@ -982,11 +982,11 @@ public class TestStoreFile extends HBaseTestCase { reader = hsf.createReader(); reader.close(cacheConf.shouldEvictOnClose()); - // We should have 3 new evictions + // We should have 3 new evictions but the evict count stat should not change. Eviction because + // of HFile invalidation is not counted along with normal evictions assertEquals(startHit, cs.getHitCount()); assertEquals(startMiss, cs.getMissCount()); - assertEquals(startEvicted + 3, cs.getEvictedCount()); - startEvicted += 3; + assertEquals(startEvicted, cs.getEvictedCount()); // Let's close the second file with evict on close turned off conf.setBoolean("hbase.rs.evictblocksonclose", false);
