-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBASE-28600 Introduce hfile.block.cache.memory.size configuration #6422
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ndimiduk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good. I have just some small comments. Have you tried it out? Is it working for you?
| if (usage != null) { | ||
| max = usage.getMax(); | ||
| } | ||
| final long heapMax = usage != null ? usage.getMax() : -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we throw when heapMax == -1? We don't have any safety on subsequent divide by -1, below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of immediately returning -1 in this case?
We expect this method to return a negative value when the cache is disabled.
hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/BlockCacheFactory.java
Lines 117 to 120 in 1880cb3
| final long cacheSize = MemorySizeUtil.getOnHeapCacheSize(c); | |
| if (cacheSize < 0) { | |
| return null; | |
| } |
| 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 + " -> " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: please cleanup any modified log lines to use modern style parameter substitution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. updated.
jinhyukify
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried it out? Is it working for you?
I briefly wrote down the developer test I performed on my local machine.
You can check it in this comment
| 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 + " -> " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. updated.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
d5d85df to
a401560
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ndimiduk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice thank you!
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
…6422) Signed-off-by: Nick Dimiduk <[email protected]>
…6422) Signed-off-by: Nick Dimiduk <[email protected]>
|
I think that this change is benign enough for our active release lines. @apurtell do you have any objection to adding this to branch-2.5? |
…6422) Signed-off-by: Nick Dimiduk <[email protected]>
…6529) Signed-off-by: Nick Dimiduk <[email protected]> Co-authored-by: JinHyuk Kim <[email protected]>
…6422) Signed-off-by: Nick Dimiduk <[email protected]>
#6422)" to branch-2 (#6544) Signed-off-by: Andrew Purtell <[email protected]>
#6422)" to branch-2.6 (#6545) Signed-off-by: Andrew Purtell <[email protected]>
#6422)" to branch-2.5 (#6592) Signed-off-by: Andrew Purtell <[email protected]>
apache#6422)" to branch-2 (apache#6544) Signed-off-by: Andrew Purtell <[email protected]>
apache#6422)" to branch-2.5 (apache#6592) Signed-off-by: Andrew Purtell <[email protected]>
Introduce a new configuration,
hfile.block.cache.memory.size, which allows setting the block cache size with an exact byte value or a human-readable storage size.