HBASE-16650 Wrong usage of BlockCache eviction stat for heap memory tuning.

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.";

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) {
+  public static LruBlockCache getL1(final Configuration c) {
-    long lruCacheSize = getLruCacheSize(c, mu);
+    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();
+    long xmx = ManagementFactory.getMemoryMXBean().getHeapMemoryUsage().getMax();
-    LruBlockCache l1 = getL1(conf, mu);
+    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;
+  }
 }

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) {
-    if (evictedByEvictionProcess && victimHandler != null) {
+      // When the eviction of the block happened because of invalidation of HFiles, no need to
-      if (victimHandler instanceof BucketCache) {
+      // update the stats counter.
-        boolean wait = getCurrentSize() < acceptableSize();
+      stats.evicted(block.getCachedTime(), block.getCacheKey().isPrimary());
-        boolean inMemory = block.getPriority() == BlockPriority.MEMORY;
+      if (victimHandler != null) {
-        ((BucketCache)victimHandler).cacheBlockWithWait(block.getCacheKey(), block.getBuffer(),
+        if (victimHandler instanceof BucketCache) {
-            inMemory, wait);
+          boolean wait = getCurrentSize() < acceptableSize();
-      } else {
+          boolean inMemory = block.getPriority() == BlockPriority.MEMORY;
-        victimHandler.cacheBlock(block.getCacheKey(), block.getBuffer());
+          ((BucketCache) victimHandler).cacheBlockWithWait(block.getCacheKey(), block.getBuffer(),
+              inMemory, wait);
+        } else {
+          victimHandler.cacheBlock(block.getCacheKey(), block.getBuffer());
+        }
       }
     }
     return block.heapSize();

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);
+    ResizableBlockCache l1Cache = CacheConfig.getL1(conf);
-    if (blockCache instanceof ResizableBlockCache) {
+    if (l1Cache != null) {
-      return new HeapMemoryManager((ResizableBlockCache) blockCache, memStoreFlusher, server,
+      return new HeapMemoryManager(l1Cache, memStoreFlusher, server, regionServerAccounting);
-                 regionServerAccounting);
     }
     return null;
   }

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) {

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);

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();
   }
 

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;
   }
 

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() {

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());
+    assertEquals(startEvicted, cs.getEvictedCount());
-    startEvicted += 3;
 
     // Let's close the second file with evict on close turned off
     conf.setBoolean("hbase.rs.evictblocksonclose", false);