HBASE-26510 Separate the blockcache enabled key and cache data on read key

hbase-client/src/main/java/org/apache/hadoop/hbase/client/ColumnFamilyDescriptor.java

@@ -210,6 +210,12 @@ public interface ColumnFamilyDescriptor {
    * and BLOOM type blocks).
    */
   boolean isBlockCacheEnabled();
+
+  /**
+   * @return true if we should cache data blocks on read
+   */
+  boolean isCacheDataOnRead();
+
   /**
    * @return true if we should cache bloomfilter blocks on write
    */

hbase-client/src/main/java/org/apache/hadoop/hbase/client/ColumnFamilyDescriptorBuilder.java

@@ -92,6 +92,9 @@ public class ColumnFamilyDescriptorBuilder {
   public static final String BLOCKCACHE = "BLOCKCACHE";
   private static final Bytes BLOCKCACHE_BYTES = new Bytes(Bytes.toBytes(BLOCKCACHE));
   @InterfaceAudience.Private
+  public static final String CACHE_DATA_ON_READ = "CACHE_DATA_ON_READ";
+  private static final Bytes CACHE_DATA_ON_READ_BYTES = new Bytes(Bytes.toBytes(CACHE_DATA_ON_READ));
+  @InterfaceAudience.Private
   public static final String CACHE_DATA_ON_WRITE = "CACHE_DATA_ON_WRITE";
   private static final Bytes CACHE_DATA_ON_WRITE_BYTES = new Bytes(Bytes.toBytes(CACHE_DATA_ON_WRITE));
   @InterfaceAudience.Private
@@ -218,6 +221,12 @@ public class ColumnFamilyDescriptorBuilder {
    */
   public static final boolean DEFAULT_BLOCKCACHE = true;
 
+  /**
+   * Default setting for whether to cache data blocks on read if block caching
+   * is enabled.
+   */
+  public static final boolean DEFAULT_CACHE_DATA_ON_READ = true;
+
   /**
    * Default setting for whether to cache data blocks on write if block caching
    * is enabled.
@@ -296,6 +305,7 @@ public static Map<String, String> getDefaultValues() {
     DEFAULT_VALUES.put(BLOCKSIZE, String.valueOf(DEFAULT_BLOCKSIZE));
     DEFAULT_VALUES.put(IN_MEMORY, String.valueOf(DEFAULT_IN_MEMORY));
     DEFAULT_VALUES.put(BLOCKCACHE, String.valueOf(DEFAULT_BLOCKCACHE));
+    DEFAULT_VALUES.put(CACHE_DATA_ON_READ, String.valueOf(DEFAULT_CACHE_DATA_ON_READ));
     DEFAULT_VALUES.put(KEEP_DELETED_CELLS, String.valueOf(DEFAULT_KEEP_DELETED));
     DEFAULT_VALUES.put(DATA_BLOCK_ENCODING, String.valueOf(DEFAULT_DATA_BLOCK_ENCODING));
     // Do NOT add this key/value by default. NEW_VERSION_BEHAVIOR is NOT defined in hbase1 so
@@ -433,6 +443,11 @@ public ColumnFamilyDescriptorBuilder setBloomFilterType(final BloomType value) {
     return this;
   }
 
+  public ColumnFamilyDescriptorBuilder setCacheDataOnRead(boolean value) {
+    desc.setCacheDataOnRead(value);
+    return this;
+  }
+
   public ColumnFamilyDescriptorBuilder setCacheBloomsOnWrite(boolean value) {
     desc.setCacheBloomsOnWrite(value);
     return this;
@@ -1038,6 +1053,20 @@ public ModifyableColumnFamilyDescriptor setScope(int scope) {
       return setValue(REPLICATION_SCOPE_BYTES, Integer.toString(scope));
     }
 
+    @Override
+    public boolean isCacheDataOnRead() {
+      return getStringOrDefault(CACHE_DATA_ON_READ_BYTES, Boolean::valueOf,
+        DEFAULT_CACHE_DATA_ON_READ);
+    }
+
+    /**
+     * @param value true if we should cache data blocks on read
+     * @return this (for chained invocation)
+     */
+    public ModifyableColumnFamilyDescriptor setCacheDataOnRead(boolean value) {
+      return setValue(CACHE_DATA_ON_READ, Boolean.toString(value));
+    }
+
     @Override
     public boolean isCacheDataOnWrite() {
       return getStringOrDefault(CACHE_DATA_ON_WRITE_BYTES, Boolean::valueOf, DEFAULT_CACHE_DATA_ON_WRITE);

hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestColumnFamilyDescriptorBuilder.java

@@ -219,7 +219,7 @@ public void testSetBlocksize() throws HBaseException {
   @Test
   public void testDefaultBuilder() {
     final Map<String, String> defaultValueMap = ColumnFamilyDescriptorBuilder.getDefaultValues();
-    assertEquals(defaultValueMap.size(), 11);
+    assertEquals(defaultValueMap.size(), 12);
     assertEquals(defaultValueMap.get(ColumnFamilyDescriptorBuilder.BLOOMFILTER),
       BloomType.ROW.toString());
     assertEquals(defaultValueMap.get(ColumnFamilyDescriptorBuilder.REPLICATION_SCOPE), "0");

hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CacheConfig.java

@@ -40,8 +40,12 @@ public class CacheConfig {
   public static final CacheConfig DISABLED = new CacheConfig();
 
   /**
-   * Configuration key to cache data blocks on read. Bloom blocks and index blocks are always be
-   * cached if the block cache is enabled.
+   * Configuration key to turn on block cache. There are separate switches for read and write.
+   */
+  public static final String BLOCKCACHE_ENABLED = "hbase.block.enabled";
+
+  /**
+   * Configuration key to cache data blocks on read.
    */
   public static final String CACHE_DATA_ON_READ_KEY = "hbase.block.data.cacheonread";
 
@@ -96,6 +100,7 @@ public class CacheConfig {
       "hbase.hfile.drop.behind.compaction";
 
   // Defaults
+  public static final boolean DEFAULT_BLOCKCACHE_ENABLED = true;
   public static final boolean DEFAULT_CACHE_DATA_ON_READ = true;
   public static final boolean DEFAULT_CACHE_DATA_ON_WRITE = false;
   public static final boolean DEFAULT_IN_MEMORY = false;
@@ -114,11 +119,14 @@ public class CacheConfig {
    * If off we will STILL cache meta blocks; i.e. INDEX and BLOOM types.
    * This cannot be disabled.
    */
-  private final boolean cacheDataOnRead;
+  private final boolean blockCacheEnabled;
 
   /** Whether blocks should be flagged as in-memory when being cached */
   private final boolean inMemory;
 
+  /** Whether data blocks should be cached when new files are read */
+  private boolean cacheDataOnRead;
+
   /** Whether data blocks should be cached when new files are written */
   private boolean cacheDataOnWrite;
 
@@ -175,15 +183,17 @@ public CacheConfig(Configuration conf, BlockCache blockCache) {
    */
   public CacheConfig(Configuration conf, ColumnFamilyDescriptor family, BlockCache blockCache,
       ByteBuffAllocator byteBuffAllocator) {
-    this.cacheDataOnRead = conf.getBoolean(CACHE_DATA_ON_READ_KEY, DEFAULT_CACHE_DATA_ON_READ) &&
-        (family == null ? true : family.isBlockCacheEnabled());
+    this.blockCacheEnabled = conf.getBoolean(BLOCKCACHE_ENABLED, DEFAULT_BLOCKCACHE_ENABLED) &&
+        (family == null || family.isBlockCacheEnabled());
     this.inMemory = family == null ? DEFAULT_IN_MEMORY : family.isInMemory();
     this.cacheDataCompressed =
         conf.getBoolean(CACHE_DATA_BLOCKS_COMPRESSED_KEY, DEFAULT_CACHE_DATA_COMPRESSED);
     this.dropBehindCompaction =
         conf.getBoolean(DROP_BEHIND_CACHE_COMPACTION_KEY, DROP_BEHIND_CACHE_COMPACTION_DEFAULT);
     // For the following flags we enable them regardless of per-schema settings
     // if they are enabled in the global configuration.
+    this.cacheDataOnRead = conf.getBoolean(CACHE_DATA_ON_READ_KEY, DEFAULT_CACHE_DATA_ON_READ) ||
+      (family != null && family.isCacheDataOnRead());
     this.cacheDataOnWrite =
         conf.getBoolean(CACHE_BLOCKS_ON_WRITE_KEY, DEFAULT_CACHE_DATA_ON_WRITE) ||
             (family == null ? false : family.isCacheDataOnWrite());
@@ -209,8 +219,9 @@ public CacheConfig(Configuration conf, ColumnFamilyDescriptor family, BlockCache
    * @param cacheConf
    */
   public CacheConfig(CacheConfig cacheConf) {
-    this.cacheDataOnRead = cacheConf.cacheDataOnRead;
+    this.blockCacheEnabled = cacheConf.blockCacheEnabled;
     this.inMemory = cacheConf.inMemory;
+    this.cacheDataOnRead = cacheConf.cacheDataOnRead;
     this.cacheDataOnWrite = cacheConf.cacheDataOnWrite;
     this.cacheIndexesOnWrite = cacheConf.cacheIndexesOnWrite;
     this.cacheBloomsOnWrite = cacheConf.cacheBloomsOnWrite;
@@ -225,8 +236,9 @@ public CacheConfig(CacheConfig cacheConf) {
   }
 
   private CacheConfig() {
-    this.cacheDataOnRead = false;
+    this.blockCacheEnabled = false;
     this.inMemory = false;
+    this.cacheDataOnRead = false;
     this.cacheDataOnWrite = false;
     this.cacheIndexesOnWrite = false;
     this.cacheBloomsOnWrite = false;
@@ -245,7 +257,7 @@ private CacheConfig() {
    * @return true if blocks should be cached on read, false if not
    */
   public boolean shouldCacheDataOnRead() {
-    return cacheDataOnRead;
+    return blockCacheEnabled && cacheDataOnRead;
   }
 
   public boolean shouldDropBehindCompaction() {
@@ -258,8 +270,10 @@ public boolean shouldDropBehindCompaction() {
    * available.
    */
   public boolean shouldCacheBlockOnRead(BlockCategory category) {
-    return cacheDataOnRead || category == BlockCategory.INDEX || category == BlockCategory.BLOOM ||
-        (prefetchOnOpen && (category != BlockCategory.META && category != BlockCategory.UNKNOWN));
+    return blockCacheEnabled &&
+      (cacheDataOnRead || category == BlockCategory.INDEX || category == BlockCategory.BLOOM ||
+        category == BlockCategory.META) ||
+      (prefetchOnOpen && (category != BlockCategory.META && category != BlockCategory.UNKNOWN));
   }
 
   /**
@@ -285,6 +299,14 @@ public void setCacheDataOnWrite(boolean cacheDataOnWrite) {
     this.cacheDataOnWrite = cacheDataOnWrite;
   }
 
+  /**
+   * @param cacheDataOnRead whether data blocks should be written to the cache
+   *                        when an HFile is read.
+   */
+  public void setCacheDataOnRead(boolean cacheDataOnRead) {
+    this.cacheDataOnRead = cacheDataOnRead;
+  }
+
   /**
    * Enable cache on write including:
    * cacheDataOnWrite
@@ -334,7 +356,7 @@ public void setEvictOnClose(boolean evictOnClose) {
    * @return true if data blocks should be compressed in the cache, false if not
    */
   public boolean shouldCacheDataCompressed() {
-    return this.cacheDataOnRead && this.cacheDataCompressed;
+    return this.blockCacheEnabled && this.cacheDataCompressed;
   }
 
   /**
@@ -343,7 +365,7 @@ public boolean shouldCacheDataCompressed() {
   public boolean shouldCacheCompressed(BlockCategory category) {
     switch (category) {
       case DATA:
-        return this.cacheDataOnRead && this.cacheDataCompressed;
+        return this.blockCacheEnabled && this.cacheDataCompressed;
       default:
         return false;
     }
@@ -371,30 +393,9 @@ public long getCacheCompactedBlocksOnWriteThreshold() {
   }
   /**
    * Return true if we may find this type of block in block cache.
-   * <p>
-   * TODO: today {@code family.isBlockCacheEnabled()} only means {@code cacheDataOnRead}, so here we
-   * consider lots of other configurations such as {@code cacheDataOnWrite}. We should fix this in
-   * the future, {@code cacheDataOnWrite} should honor the CF level {@code isBlockCacheEnabled}
-   * configuration.
    */
   public boolean shouldReadBlockFromCache(BlockType blockType) {
-    if (cacheDataOnRead) {
-      return true;
-    }
-    if (prefetchOnOpen) {
-      return true;
-    }
-    if (cacheDataOnWrite) {
-      return true;
-    }
-    if (blockType == null) {
-      return true;
-    }
-    if (blockType.getCategory() == BlockCategory.BLOOM ||
-        blockType.getCategory() == BlockCategory.INDEX) {
-      return true;
-    }
-    return false;
+    return blockCacheEnabled;
   }
 
   /**

hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheConfig.java

@@ -204,6 +204,20 @@ public void testDisableCacheDataBlock() throws IOException {
     assertFalse(cacheConfig.shouldCacheBloomsOnWrite());
     assertFalse(cacheConfig.shouldCacheIndexesOnWrite());
 
+    conf.setBoolean(CacheConfig.CACHE_DATA_ON_READ_KEY, false);
+    cacheConfig = new CacheConfig(conf);
+    assertFalse(cacheConfig.shouldCacheBlockOnRead(BlockCategory.DATA));
+    assertFalse(cacheConfig.shouldCacheCompressed(BlockCategory.DATA));
+    assertFalse(cacheConfig.shouldCacheDataCompressed());
+    assertFalse(cacheConfig.shouldCacheDataOnWrite());
+    assertFalse(cacheConfig.shouldCacheDataOnRead());
+    assertTrue(cacheConfig.shouldCacheBlockOnRead(BlockCategory.INDEX));
+    assertTrue(cacheConfig.shouldCacheBlockOnRead(BlockCategory.META));
+    assertTrue(cacheConfig.shouldCacheBlockOnRead(BlockCategory.BLOOM));
+    assertFalse(cacheConfig.shouldCacheBloomsOnWrite());
+    assertFalse(cacheConfig.shouldCacheIndexesOnWrite());
+
+    conf.setBoolean(CacheConfig.CACHE_DATA_ON_READ_KEY, true);
     conf.setBoolean(CacheConfig.CACHE_BLOCKS_ON_WRITE_KEY, true);
     conf.setBoolean(CacheConfig.CACHE_DATA_BLOCKS_COMPRESSED_KEY, true);
     conf.setBoolean(CacheConfig.CACHE_BLOOM_BLOCKS_ON_WRITE_KEY, true);
@@ -221,7 +235,7 @@ public void testDisableCacheDataBlock() throws IOException {
     assertTrue(cacheConfig.shouldCacheBloomsOnWrite());
     assertTrue(cacheConfig.shouldCacheIndexesOnWrite());
 
-    conf.setBoolean(CacheConfig.CACHE_DATA_ON_READ_KEY, false);
+    conf.setBoolean(CacheConfig.BLOCKCACHE_ENABLED, false);
     conf.setBoolean(CacheConfig.CACHE_BLOCKS_ON_WRITE_KEY, false);
 
     cacheConfig = new CacheConfig(conf);
@@ -230,13 +244,13 @@ public void testDisableCacheDataBlock() throws IOException {
     assertFalse(cacheConfig.shouldCacheDataCompressed());
     assertFalse(cacheConfig.shouldCacheDataOnWrite());
     assertFalse(cacheConfig.shouldCacheDataOnRead());
-    assertTrue(cacheConfig.shouldCacheBlockOnRead(BlockCategory.INDEX));
+    assertFalse(cacheConfig.shouldCacheBlockOnRead(BlockCategory.INDEX));
     assertFalse(cacheConfig.shouldCacheBlockOnRead(BlockCategory.META));
-    assertTrue(cacheConfig.shouldCacheBlockOnRead(BlockCategory.BLOOM));
+    assertFalse(cacheConfig.shouldCacheBlockOnRead(BlockCategory.BLOOM));
     assertTrue(cacheConfig.shouldCacheBloomsOnWrite());
     assertTrue(cacheConfig.shouldCacheIndexesOnWrite());
 
-    conf.setBoolean(CacheConfig.CACHE_DATA_ON_READ_KEY, true);
+    conf.setBoolean(CacheConfig.BLOCKCACHE_ENABLED, true);
     conf.setBoolean(CacheConfig.CACHE_BLOCKS_ON_WRITE_KEY, false);
 
     ColumnFamilyDescriptor columnFamilyDescriptor =
@@ -251,9 +265,9 @@ public void testDisableCacheDataBlock() throws IOException {
     assertFalse(cacheConfig.shouldCacheDataCompressed());
     assertFalse(cacheConfig.shouldCacheDataOnWrite());
     assertFalse(cacheConfig.shouldCacheDataOnRead());
-    assertTrue(cacheConfig.shouldCacheBlockOnRead(BlockCategory.INDEX));
+    assertFalse(cacheConfig.shouldCacheBlockOnRead(BlockCategory.INDEX));
     assertFalse(cacheConfig.shouldCacheBlockOnRead(BlockCategory.META));
-    assertTrue(cacheConfig.shouldCacheBlockOnRead(BlockCategory.BLOOM));
+    assertFalse(cacheConfig.shouldCacheBlockOnRead(BlockCategory.BLOOM));
     assertTrue(cacheConfig.shouldCacheBloomsOnWrite());
     assertTrue(cacheConfig.shouldCacheIndexesOnWrite());
   }

hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestForceCacheImportantBlocks.java

@@ -18,6 +18,7 @@
 package org.apache.hadoop.hbase.io.hfile;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
 import java.io.IOException;
@@ -123,12 +124,14 @@ public void testCacheBlocks() throws IOException {
     // Do a single get, take count of caches.  If we are NOT caching DATA blocks, the miss
     // count should go up.  Otherwise, all should be cached and the miss count should not rise.
     region.get(new Get(Bytes.toBytes("row" + 0)));
-    assertTrue(stats.getHitCount() > 0);
+    // If we do not cache any blocks, the hit count should be 0.
+    assertFalse(stats.getHitCount() > 0 ^ this.cfCacheEnabled);
     assertTrue(HFile.DATABLOCK_READ_COUNT.sum() > 0);
     long missCount = stats.getMissCount();
     region.get(new Get(Bytes.toBytes("row" + 0)));
     if (this.cfCacheEnabled) assertEquals(missCount, stats.getMissCount());
-    else assertTrue(stats.getMissCount() > missCount);
+    // If we disable the cache, the scanner will not search cache, so that the missCount is still.
+    else assertEquals(stats.getMissCount(), missCount);
   }
 
   private void writeTestData(HRegion region) throws IOException {