Re: [PR] HBASE-28600 Introduce hfile.block.cache.memory.size configuration [hbase]

["ndimiduk (via GitHub)" <gi...@apache.org>]

ndimiduk commented on code in PR #6422:
URL: https://github.com/apache/hbase/pull/6422#discussion_r1872959395


##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/util/MemorySizeUtil.java:
##########
@@ -207,27 +228,27 @@ public static float getBlockCacheHeapPercent(final Configuration conf) {
    * @throws IllegalArgumentException if HFILE_BLOCK_CACHE_SIZE_KEY is > 1.0
    */
   public static long getOnHeapCacheSize(final Configuration conf) {
-    float cachePercentage = conf.getFloat(HConstants.HFILE_BLOCK_CACHE_SIZE_KEY,
-      HConstants.HFILE_BLOCK_CACHE_SIZE_DEFAULT);
+    final float cachePercentage = getBlockCacheHeapPercent(conf);
     if (cachePercentage <= 0.0001f) {
       return -1;
     }
     if (cachePercentage > 1.0) {
       throw new IllegalArgumentException(
         HConstants.HFILE_BLOCK_CACHE_SIZE_KEY + " must be between 0.0 and 1.0, and not > 1.0");
     }
-    long max = -1L;
+
     final MemoryUsage usage = safeGetHeapMemoryUsage();
-    if (usage != null) {
-      max = usage.getMax();
-    }
+    final long heapMax = usage != null ? usage.getMax() : -1;

Review Comment:
   Should we throw when `heapMax == -1`? We don't have any safety on subsequent divide by -1, below. 



##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HeapMemoryManager.java:
##########
@@ -161,14 +161,16 @@ private boolean doInit(Configuration conf) {
     blockCachePercentMaxRange = conf.getFloat(BLOCK_CACHE_SIZE_MAX_RANGE_KEY, blockCachePercent);
     if (blockCachePercent < blockCachePercentMinRange) {
       LOG.warn("Setting " + BLOCK_CACHE_SIZE_MIN_RANGE_KEY + " to " + blockCachePercent
-        + ", same value as " + HFILE_BLOCK_CACHE_SIZE_KEY
+        + " (lookup order: " + HFILE_BLOCK_CACHE_MEMORY_SIZE_KEY + " -> "

Review Comment:
   nit: please cleanup any modified log lines to use modern style parameter substitution.



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