Repository: hbase
Updated Branches:
  refs/heads/master 4bb84f7d0 -> 1384c9a08


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

Reply via email to