ndimiduk commented on code in PR #7726:
URL: https://github.com/apache/hbase/pull/7726#discussion_r2883521450
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/util/MemorySizeUtil.java:
##########
@@ -151,10 +154,29 @@ public static float
getRegionServerMinFreeHeapFraction(final Configuration conf)
/**
* Retrieve global memstore configured size as percentage of total heap.
*/
- public static float getGlobalMemStoreHeapPercent(final Configuration c,
+ public static float getGlobalMemStoreHeapPercent(final Configuration conf,
final boolean logInvalid) {
+ // Check if an explicit memstore size is configured.
+ long memStoreSizeInBytes = getMemstoreSizeInBytes(conf);
Review Comment:
You're still logging, even when the feature is completely unused. This will
result in a warn line produced by default. I think you can drop the `else if`
branch entirely, or guard it on `conf.get(MEMSTORE_MEMORY_SIZE_KEY) != null`.
The else if (logInvalid) branch still fires when MEMSTORE_MEMORY_SIZE_KEY is
not set at all — every RegionServer that doesn't use this feature will log a
warning about "-1 bytes is invalid" on startup.
The existing `getBlockCacheHeapPercent` in the same file behaves more like I
would expect — it's the same pattern of an optional byte-based key with
fallback to fraction:
```
long l1CacheSizeInBytes = getBlockCacheSizeInBytes(conf);
if (l1CacheSizeInBytes > 0) {
// use byte-based config
}
// silently fall through to fraction-based
return conf.getFloat(HFILE_BLOCK_CACHE_SIZE_KEY, ...);
```
No warning when the key is absent. The warning should only fire when the key
IS set but produces an out-of-range value.
--
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]
