HBASE-15625 Make minimum free heap memory percentage configurable #7076
Conversation
This comment has been minimized.
This comment has been minimized.
… lower default to 0.1
hbase-server/src/main/java/org/apache/hadoop/hbase/io/util/MemorySizeUtil.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java
Outdated
Show resolved
Hide resolved
| */ | ||
| public static final float HBASE_CLUSTER_MINIMUM_MEMORY_THRESHOLD = 0.2f; | ||
| public static final String HBASE_REGION_SERVER_FREE_HEAP_MEMORY_MIN_SIZE_KEY = | ||
| "hbase.regionserver.free.heap.memory.min.size"; |
There was a problem hiding this comment.
It's not immediately clear to me that heap.min.size expects a fraction, whereas heap.memory.min.size expects an actual size. But I don't really have a better idea, and I can imagine you had a hard time coming up with those names anyway, given that some existing .size parameters expect an actual size, and some other .size parameters expect a fraction.
That said, do you think we need both parameters? Wouldn't the actual size (which takes precedence over the fraction value) be enough in most cases?
There was a problem hiding this comment.
I realize I didn't provide enough context earlier, sorry about that.
The naming of this configuration follows the convention used by the existing block cache size configurations. There are currently two settings that determine the size of the block cache: (HBASE-28600)
hfile.block.cache.sizehfile.block.cache.memory.size
While users are generally familiar with the fraction-based approach, I thought it would be helpful to introduce an alternative that accepts human-readable memory sizes (e.g., 512m, 4g). So both options are now available.
At least for memory-related configurations, such as free heap, memstore, and block cache, I believe supporting both styles (fraction and absolute size) makes sense for the near future. Would love to hear your thoughts!
(For now, this dual option is only available for block cache, but I’m planning to extend it to memstore configuration soon as well.)
There was a problem hiding this comment.
No problem. Thanks for clarification.
hfile.block.cache.sizehfile.block.cache.memory.size
Maybe it's just me, but when I see this pair, I get the impression that size expects a fraction, whereas memory.size expects an actual value. And that made me wonder whether the absolute version of hbase.regionserver.free.heap.min.size should be named hbase.regionserver.free.heap.min.memory.size to follow the same size vs memory.size convention. But maybe there was no such convention in the first place?
I believe supporting both styles (fraction and absolute size) makes sense for the near future. Would love to hear your thoughts!
I'm not the one to make the call here, but personally, I'm not a fan of adding options just for completeness' sake, because increasing number of options tends to increase cognitive burden of the users, and as the number of possible combinations of options increases, it gets harder to reason about their combined effects. So I wanted to make sure if we definitely needed both. However, we have to admit that the number of configuration parameters of HBase is already out of control, so my point here is probably moot, because not adding a single parameter wouldn't make much difference to the project.
Anyway, the reason you and the initial reporter of the issue wanted this to be configurable was to reserve as much memory as possible for memstores and block cache when using large heap sizes. And I imagine you are more likely to use the actual size version than the fraction one, to better deal with the systems with different heap sizes and keep the safety net.
For example, let's say you have these:
- memstore = 0.6
- blockcache = 0.3
- minimum unreserved size = 10GB
You can use these values across systems with different heap sizes and still be sure that at least 10GB of the heap is unreserved. However, if you use the fraction version instead and set it to 0.9, there is no guarantee of the actual unreserved size, and that is not very different from just removing the check.
So could you explain how you are going to use these parameters on your clusters?
There was a problem hiding this comment.
Thanks for the detailed insights 😄
And that made me wonder whether the absolute version of hbase.regionserver.free.heap.min.size should be named hbase.regionserver.free.heap.min.memory.size to follow the same size vs memory.size convention. But maybe there was no such convention in the first place?
You're right. There wasn't a clear convention initially, but following the existing blockCache configuration and using the memory.size suffix seems like a better approach.
So could you explain how you are going to use these parameters on your clusters?
I was planning to use an absolute value for the heap size as well.
The reason I first added the fraction-based version was because the original config was already managed as a fraction.
But I agree with your point. Using an absolute size is not only more explicit for users, but also provides more stability.
Unlike memStore or blockCache, where we're allocating memory, this one is about preserving a minimum free heap, so an absolute value feels more appropriate.
If we go with absolute size only, we still need to think about the default value.
I typically use at least 31GB heap per RegionServer, but for low-spec VMs (e.g. 4GB heap), something like 512m might make more sense? To avoid startup issues in smaller clusters, I’m leaning toward a conservative default for now.
I'm also considering allowing this value to be disabled (by setting it to 0) or overridden to something like 128m in conf/hbase-site.xml, especially for test environments.
Do you have any thoughts on this?
There was a problem hiding this comment.
Using an absolute size is not only more explicit for users, but also provides more stability.
Thanks for giving my suggestion some thought. Considering the use cases and requirements, I think it's enough to offer a single parameter for setting the minimum amount of heap that should be left unreserved. Anyone who wants to bypass the check can set it to 0, and those who want to have some safety guarantee can set it to some realistic value. I believe it covers all scenarios, and having another option might distract/confuse the users.
But that's just one perspective, and I hope to hear what others think as well.
If we go with absolute size only, we still need to think about the default value.
Yeah, it's important to provide sensible defaults, but we should be extra careful when changing the existing ones or introducing one that might impact the existing use cases. Following "the principal of least surprise", I would just leave the value undefined, which means that 20% of the heap is used as the threshold as before.
I'm also considering allowing this value to be disabled (by setting it to 0)
We should still allow 0 as a valid value for the option, for users who want to bypass the check entirely. An empty value or a negative value can be used instead to indicate an undefined state.
overridden to something like 128m in conf/hbase-site.xml, especially for test environments.
While I think it's reasonable, I'm not confident that it won't impact existing user setups. In pseudo-distributed environments, users often run many processes locally, each with a relatively small heap (e.g., 512MB). In such cases, even 128m could cause their setup to stop working.
There was a problem hiding this comment.
I agreed with your suggestion and updated the code accordingly.
Now the configuration uses an absolute amount for setting the free heap instead of the previous fraction-based approach.
If no configuration is provided, it falls back to the default value of 0.2.
The commit is 3d2b641
Not sure what others will think, but feel free to take a look and share your thoughts.
Thanks again for your helpful input!
This comment has been minimized.
This comment has been minimized.
jinhyukify
changed the title
This comment has been minimized.
This comment has been minimized.
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHeapMemoryManager.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
hbase-server/src/main/java/org/apache/hadoop/hbase/io/util/MemorySizeUtil.java
Outdated
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/io/util/MemorySizeUtil.java
Outdated
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HeapMemoryManager.java
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HeapMemoryManager.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a configurable minimum free heap requirement for HBase RegionServers, replacing the fixed 20% threshold and updating related validation and default configuration.
- Added
hbase.regionserver.free.heap.min.memory.sizeconfig key and APIs to compute and enforce it. - Refactored
MemorySizeUtilandHeapMemoryManagerto use the new validation logic. - Updated
hbase-default.xml, unit tests, and addedmockito-inlinedependency for static mocking.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| hbase-server/src/main/java/org/apache/hadoop/hbase/io/util/MemorySizeUtil.java | Added validateRegionServerHeapMemoryAllocation, new config key, fraction/byte helpers, removed old logic |
| hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HeapMemoryManager.java | Removed hardcoded threshold, wired in new validation and limit checks |
| hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java | Replaced old free-heap check with new validation API |
| hbase-server/src/test/java/org/apache/hadoop/hbase/io/util/TestMemorySizeUtil.java | New tests for configurable free-heap behavior |
| hbase-server/pom.xml | Added mockito-inline for static method mocking |
| hbase-common/src/main/resources/hbase-default.xml | Documented new hbase.regionserver.free.heap.min.memory.size property |
| hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java | Clarified default free-heap threshold constant documentation |
Comments suppressed due to low confidence (1)
hbase-server/src/main/java/org/apache/hadoop/hbase/io/util/MemorySizeUtil.java:124
- The Javadoc references a non-existent constant (
HBASE_REGION_SERVER_FREE_HEAP_MEMORY_MIN_SIZE_KEY) and misleadingly claims an exception is thrown. Update it toHBASE_REGION_SERVER_FREE_HEAP_MIN_MEMORY_SIZE_KEYor adjust to reflect actual behavior.
* @throws IllegalArgumentException if HBASE_REGION_SERVER_FREE_HEAP_MEMORY_MIN_SIZE_KEY format is
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HeapMemoryManager.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
hbase-server/src/test/java/org/apache/hadoop/hbase/io/util/TestMemorySizeUtil.java
Outdated
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
| throw new RuntimeException(String.format( | ||
| "RegionServer heap memory allocation is invalid: " | ||
| + "memStore=%d%%, blockCache=%d%%, requiredFreeHeap=%d%%. " | ||
| + "Combined usage %d%% exceeds allowed maximum %d%%. " |
There was a problem hiding this comment.
When the hbase.regionserver.free.heap.min.memory.size is set to an exceedingly large value (possibly due to a mistake), the message becomes a little strange.
conf.setFloat(MemorySizeUtil.MEMSTORE_SIZE_KEY, 0.5f);
conf.setFloat(HConstants.HFILE_BLOCK_CACHE_SIZE_KEY, 0.6f);
conf.set(MemorySizeUtil.HBASE_REGION_SERVER_FREE_HEAP_MIN_MEMORY_SIZE_KEY, "100g");
MemorySizeUtil.validateRegionServerHeapMemoryAllocation(conf);RegionServer heap memory allocation is invalid: memStore=50%, blockCache=60%, requiredFreeHeap=4654%.
Combined usage 110% exceeds allowed maximum -4554%.
Maybe it's better to simplify the message to something like Sum exceeds 100%.
| @@ -77,34 +82,72 @@ 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, | |||
There was a problem hiding this comment.
nit.
| * Validates that heap allocations for memStore and block cache do not exceed the allowed limit, | |
| * Validates that heap allocations for MemStore and block cache do not exceed the allowed limit, |
Jira https://issues.apache.org/jira/browse/HBASE-15625
In modern deployments with large heap sizes, there are cases where allocating a very large memstore aggressively can be beneficial for performance. (e.g., memstore=0.6, blockcache=0.3)
In such scenarios, keeping 20% of the heap free is unnecessarily conservative. To better accommodate these cases, I reduced the default minimum free heap threshold to 10% and made it configurable.
hbase.regionserver.free.heap.min.sizehbase.regionserver.free.heap.memory.min.sizeDefault minimum free heap threshold lowered from 20% to 10%.<-- Reverted. Please see the comment.