[GitHub] [hbase] wchevreuil commented on a diff in pull request #4805: HBASE-27365 Minimise block addition failures due to no space in bucke…

[GitBox <gi...@apache.org>]

wchevreuil commented on code in PR #4805:
URL: https://github.com/apache/hbase/pull/4805#discussion_r983418390


##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java:
##########
@@ -288,6 +292,8 @@ public BucketCache(String ioEngineName, long capacity, int blockSize, int[] buck
     this.singleFactor = conf.getFloat(SINGLE_FACTOR_CONFIG_NAME, DEFAULT_SINGLE_FACTOR);
     this.multiFactor = conf.getFloat(MULTI_FACTOR_CONFIG_NAME, DEFAULT_MULTI_FACTOR);
     this.memoryFactor = conf.getFloat(MEMORY_FACTOR_CONFIG_NAME, DEFAULT_MEMORY_FACTOR);
+    this.queueAdditionWaitTime = conf.getLong(QUEUE_ADDITION_WAIT_TIME,
+      DEFAULT_QUEUE_ADDITION_WAIT_TIME);

Review Comment:
   Any cons on setting a default value > 0?



##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruAdaptiveBlockCache.java:
##########
@@ -552,6 +552,11 @@ public void cacheBlock(BlockCacheKey cacheKey, Cacheable buf, boolean inMemory)
     }
   }
 
+  @Override public void cacheBlock(BlockCacheKey cacheKey, Cacheable buf, boolean inMemory,

Review Comment:
   nit: Make this the default impl in the interface?



##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderImpl.java:
##########
@@ -1341,8 +1341,10 @@ public HFileBlock readBlock(long dataBlockOffset, long onDiskBlockSize, final bo
         // Cache the block if necessary
         cacheConf.getBlockCache().ifPresent(cache -> {
           if (cacheBlock && cacheConf.shouldCacheBlockOnRead(category)) {
+            // Using the wait on cache during compaction and prefetching.
             cache.cacheBlock(cacheKey, cacheCompressed ? hfileBlock : unpacked,
-              cacheConf.isInMemory());
+              cacheConf.isInMemory(), isCompaction ||
+                (pread  && expectedBlockType == null && expectedDataBlockEncoding == null));

Review Comment:
   Mind explain the logic behind these particular flags to determine when we are going to trigger the wait? And we don't need this when caching compressed blocks in line#1331 above? 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org