Copilot commented on code in PR #7076:
URL: https://github.com/apache/hbase/pull/7076#discussion_r2227033245
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/util/MemorySizeUtil.java:
##########
@@ -77,34 +82,70 @@ public static MemoryUsage safeGetHeapMemoryUsage() {
}
/**
- * Checks whether we have enough heap memory left out after portion for
Memstore and Block cache.
- * We need atleast 20% of heap left out for other RS functions.
+ * Validates that heap allocations for MemStore and block cache do not
exceed the allowed limit,
+ * ensuring enough free heap remains for other RegionServer tasks.
+ * @param conf the configuration to validate
+ * @throws RuntimeException if the combined allocation exceeds the threshold
*/
- public static void checkForClusterFreeHeapMemoryLimit(Configuration conf) {
+ public static void validateRegionServerHeapMemoryAllocation(Configuration
conf) {
if (conf.get(MEMSTORE_SIZE_OLD_KEY) != null) {
LOG.warn(MEMSTORE_SIZE_OLD_KEY + " is deprecated by " +
MEMSTORE_SIZE_KEY);
}
- float globalMemstoreSize = getGlobalMemStoreHeapPercent(conf, false);
- int gml = (int) (globalMemstoreSize * CONVERT_TO_PERCENTAGE);
- float blockCacheUpperLimit = getBlockCacheHeapPercent(conf);
- int bcul = (int) (blockCacheUpperLimit * CONVERT_TO_PERCENTAGE);
- if (
- CONVERT_TO_PERCENTAGE - (gml + bcul)
- < (int) (CONVERT_TO_PERCENTAGE *
HConstants.HBASE_CLUSTER_MINIMUM_MEMORY_THRESHOLD)
- ) {
- throw new RuntimeException("Current heap configuration for MemStore and
BlockCache exceeds "
- + "the threshold required for successful cluster operation. "
- + "The combined value cannot exceed 0.8. Please check " + "the
settings for "
- + MEMSTORE_SIZE_KEY + " and either " +
HConstants.HFILE_BLOCK_CACHE_MEMORY_SIZE_KEY + " or "
- + HConstants.HFILE_BLOCK_CACHE_SIZE_KEY + " in your configuration. " +
MEMSTORE_SIZE_KEY
- + "=" + globalMemstoreSize + ", " +
HConstants.HFILE_BLOCK_CACHE_MEMORY_SIZE_KEY + "="
- + conf.get(HConstants.HFILE_BLOCK_CACHE_MEMORY_SIZE_KEY) + ", "
- + HConstants.HFILE_BLOCK_CACHE_SIZE_KEY + "="
- + conf.get(HConstants.HFILE_BLOCK_CACHE_SIZE_KEY) + ". (Note: If both "
- + HConstants.HFILE_BLOCK_CACHE_MEMORY_SIZE_KEY + " and "
- + HConstants.HFILE_BLOCK_CACHE_SIZE_KEY + " are set, " + "the system
will use "
- + HConstants.HFILE_BLOCK_CACHE_MEMORY_SIZE_KEY + ")");
+ float memStoreFraction = getGlobalMemStoreHeapPercent(conf, false);
+ float blockCacheFraction = getBlockCacheHeapPercent(conf);
+ float minFreeHeapFraction = getRegionServerMinFreeHeapFraction(conf);
+
+ int memStorePercent = (int) (memStoreFraction * 100);
+ int blockCachePercent = (int) (blockCacheFraction * 100);
+ int minFreeHeapPercent = (int) (minFreeHeapFraction * 100);
+ int usedPercent = memStorePercent + blockCachePercent;
+ int maxAllowedUsed = 100 - minFreeHeapPercent;
+
+ if (usedPercent > maxAllowedUsed) {
+ throw new RuntimeException(String.format(
+ "RegionServer heap memory allocation is invalid: total memory usage
exceeds 100%% "
+ + "(memStore + blockCache + requiredFreeHeap). "
+ + "Check the following configuration values:%n" + " - %s = %.2f%n"
+ " - %s = %s%n"
+ + " - %s = %s%n" + " - %s = %s",
+ MEMSTORE_SIZE_KEY, memStoreFraction,
HConstants.HFILE_BLOCK_CACHE_MEMORY_SIZE_KEY,
+ conf.get(HConstants.HFILE_BLOCK_CACHE_MEMORY_SIZE_KEY),
+ HConstants.HFILE_BLOCK_CACHE_SIZE_KEY,
conf.get(HConstants.HFILE_BLOCK_CACHE_SIZE_KEY),
+ HBASE_REGION_SERVER_FREE_HEAP_MIN_MEMORY_SIZE_KEY,
+ conf.get(HBASE_REGION_SERVER_FREE_HEAP_MIN_MEMORY_SIZE_KEY)));
+ }
+ }
+
+ /**
+ * Retrieve an explicit minimum required free heap size in bytes in the
configuration.
+ * @param conf used to read configs
+ * @return the minimum required free heap size in bytes, or a negative value
if not configured.
+ * @throws IllegalArgumentException if
HBASE_REGION_SERVER_FREE_HEAP_MIN_MEMORY_SIZE_KEY format is
+ * invalid
+ */
+ private static long getRegionServerMinFreeHeapInBytes(Configuration conf) {
+ final String key = HBASE_REGION_SERVER_FREE_HEAP_MIN_MEMORY_SIZE_KEY;
+ try {
+ return Long.parseLong(conf.get(key));
Review Comment:
If conf.get(key) returns null, Long.parseLong() will throw
NumberFormatException. This should be handled by checking for null first or
using a default value.
```suggestion
String value = conf.get(key);
if (value == null) {
return (long) conf.getStorageSize(key, -1, StorageUnit.BYTES);
}
try {
return Long.parseLong(value);
```
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/util/MemorySizeUtil.java:
##########
@@ -77,34 +82,70 @@ public static MemoryUsage safeGetHeapMemoryUsage() {
}
/**
- * Checks whether we have enough heap memory left out after portion for
Memstore and Block cache.
- * We need atleast 20% of heap left out for other RS functions.
+ * Validates that heap allocations for MemStore and block cache do not
exceed the allowed limit,
+ * ensuring enough free heap remains for other RegionServer tasks.
+ * @param conf the configuration to validate
+ * @throws RuntimeException if the combined allocation exceeds the threshold
*/
- public static void checkForClusterFreeHeapMemoryLimit(Configuration conf) {
+ public static void validateRegionServerHeapMemoryAllocation(Configuration
conf) {
if (conf.get(MEMSTORE_SIZE_OLD_KEY) != null) {
LOG.warn(MEMSTORE_SIZE_OLD_KEY + " is deprecated by " +
MEMSTORE_SIZE_KEY);
}
- float globalMemstoreSize = getGlobalMemStoreHeapPercent(conf, false);
- int gml = (int) (globalMemstoreSize * CONVERT_TO_PERCENTAGE);
- float blockCacheUpperLimit = getBlockCacheHeapPercent(conf);
- int bcul = (int) (blockCacheUpperLimit * CONVERT_TO_PERCENTAGE);
- if (
- CONVERT_TO_PERCENTAGE - (gml + bcul)
- < (int) (CONVERT_TO_PERCENTAGE *
HConstants.HBASE_CLUSTER_MINIMUM_MEMORY_THRESHOLD)
- ) {
- throw new RuntimeException("Current heap configuration for MemStore and
BlockCache exceeds "
- + "the threshold required for successful cluster operation. "
- + "The combined value cannot exceed 0.8. Please check " + "the
settings for "
- + MEMSTORE_SIZE_KEY + " and either " +
HConstants.HFILE_BLOCK_CACHE_MEMORY_SIZE_KEY + " or "
- + HConstants.HFILE_BLOCK_CACHE_SIZE_KEY + " in your configuration. " +
MEMSTORE_SIZE_KEY
- + "=" + globalMemstoreSize + ", " +
HConstants.HFILE_BLOCK_CACHE_MEMORY_SIZE_KEY + "="
- + conf.get(HConstants.HFILE_BLOCK_CACHE_MEMORY_SIZE_KEY) + ", "
- + HConstants.HFILE_BLOCK_CACHE_SIZE_KEY + "="
- + conf.get(HConstants.HFILE_BLOCK_CACHE_SIZE_KEY) + ". (Note: If both "
- + HConstants.HFILE_BLOCK_CACHE_MEMORY_SIZE_KEY + " and "
- + HConstants.HFILE_BLOCK_CACHE_SIZE_KEY + " are set, " + "the system
will use "
- + HConstants.HFILE_BLOCK_CACHE_MEMORY_SIZE_KEY + ")");
+ float memStoreFraction = getGlobalMemStoreHeapPercent(conf, false);
+ float blockCacheFraction = getBlockCacheHeapPercent(conf);
+ float minFreeHeapFraction = getRegionServerMinFreeHeapFraction(conf);
+
+ int memStorePercent = (int) (memStoreFraction * 100);
+ int blockCachePercent = (int) (blockCacheFraction * 100);
+ int minFreeHeapPercent = (int) (minFreeHeapFraction * 100);
+ int usedPercent = memStorePercent + blockCachePercent;
+ int maxAllowedUsed = 100 - minFreeHeapPercent;
+
+ if (usedPercent > maxAllowedUsed) {
+ throw new RuntimeException(String.format(
+ "RegionServer heap memory allocation is invalid: total memory usage
exceeds 100%% "
+ + "(memStore + blockCache + requiredFreeHeap). "
+ + "Check the following configuration values:%n" + " - %s = %.2f%n"
+ " - %s = %s%n"
+ + " - %s = %s%n" + " - %s = %s",
+ MEMSTORE_SIZE_KEY, memStoreFraction,
HConstants.HFILE_BLOCK_CACHE_MEMORY_SIZE_KEY,
+ conf.get(HConstants.HFILE_BLOCK_CACHE_MEMORY_SIZE_KEY),
+ HConstants.HFILE_BLOCK_CACHE_SIZE_KEY,
conf.get(HConstants.HFILE_BLOCK_CACHE_SIZE_KEY),
+ HBASE_REGION_SERVER_FREE_HEAP_MIN_MEMORY_SIZE_KEY,
+ conf.get(HBASE_REGION_SERVER_FREE_HEAP_MIN_MEMORY_SIZE_KEY)));
+ }
+ }
+
+ /**
+ * Retrieve an explicit minimum required free heap size in bytes in the
configuration.
+ * @param conf used to read configs
+ * @return the minimum required free heap size in bytes, or a negative value
if not configured.
+ * @throws IllegalArgumentException if
HBASE_REGION_SERVER_FREE_HEAP_MIN_MEMORY_SIZE_KEY format is
+ * invalid
+ */
+ private static long getRegionServerMinFreeHeapInBytes(Configuration conf) {
Review Comment:
The method could throw IllegalArgumentException but the exception handling
only catches NumberFormatException. If conf.getStorageSize() throws
IllegalArgumentException for invalid format, it won't be caught and the
documented exception won't be thrown as expected.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HeapMemoryManager.java:
##########
@@ -363,14 +371,15 @@ private void tune() {
+ blockCachePercentMaxRange + ". Resetting blockCacheSize to min
size");
blockCacheSize = blockCachePercentMaxRange;
}
- int gml = (int) (memstoreSize * CONVERT_TO_PERCENTAGE);
- int bcul = (int) ((blockCacheSize) * CONVERT_TO_PERCENTAGE);
- if (CONVERT_TO_PERCENTAGE - (gml + bcul) <
CLUSTER_MINIMUM_MEMORY_THRESHOLD) {
+
+ if (isHeapMemoryUsageExceedingLimit(memstoreSize, blockCacheSize)) {
LOG.info("Current heap configuration from HeapMemoryTuner exceeds "
- + "the threshold required for successful cluster operation. "
- + "The combined value cannot exceed 0.8. " +
MemorySizeUtil.MEMSTORE_SIZE_KEY + " is "
- + memstoreSize + " and " + HFILE_BLOCK_CACHE_SIZE_KEY + " is " +
blockCacheSize);
- // TODO can adjust the value so as not exceed 80%. Is that correct?
may be.
+ + "the allowed heap usage. At least " + minFreeHeapFraction
+ + " of the heap must remain free to ensure stable RegionServer
operation. "
+ + MemorySizeUtil.MEMSTORE_SIZE_KEY + " is " + memstoreSize + " and
"
+ + HFILE_BLOCK_CACHE_SIZE_KEY + " is " + blockCacheSize);
+ // NOTE: In the future, we might adjust values to not exceed limits,
+ // but for now tuning is skipped if over threshold.
Review Comment:
[nitpick] This TODO-style comment should either be converted to a proper
TODO with a tracking issue, or removed if no action is planned. Comments about
future work without clear ownership can become stale.
```suggestion
// Tuning is skipped if the current configuration exceeds the
allowed heap usage limits.
// This ensures stable RegionServer operation under the current
constraints.
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
