Skip to content

Conversation

@jinhyukify
Copy link
Contributor

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.

@Apache-HBase

This comment has been minimized.

@Apache-HBase

This comment has been minimized.

Copy link
Member

@ndimiduk ndimiduk left a 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;
Copy link
Member

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.

Copy link
Contributor Author

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.

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 + " -> "
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. updated.

Copy link
Contributor Author

@jinhyukify jinhyukify left a 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 + " -> "
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. updated.

@Apache-HBase

This comment has been minimized.

@Apache-HBase

This comment has been minimized.

@Apache-HBase

This comment has been minimized.

@Apache-HBase

This comment has been minimized.

@jinhyukify jinhyukify requested a review from ndimiduk December 9, 2024 05:31
Copy link
Member

@ndimiduk ndimiduk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice thank you!

@Apache-HBase
Copy link

🎊 +1 overall

Vote Subsystem Runtime Logfile Comment
+0 🆗 reexec 0m 31s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 0s No case conflicting files found.
+0 🆗 codespell 0m 0s codespell was not available.
+0 🆗 detsecrets 0m 0s detect-secrets was not available.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
+1 💚 hbaseanti 0m 0s Patch does not have any anti-patterns.
_ master Compile Tests _
+0 🆗 mvndep 0m 11s Maven dependency ordering for branch
+1 💚 mvninstall 3m 12s master passed
+1 💚 compile 3m 46s master passed
+1 💚 checkstyle 0m 49s master passed
+1 💚 spotbugs 2m 2s master passed
+0 🆗 refguide 2m 20s branch has no errors when building the reference guide. See footer for rendered docs, which you should manually inspect.
+1 💚 spotless 0m 46s branch has no errors when running spotless:check.
_ Patch Compile Tests _
+0 🆗 mvndep 0m 11s Maven dependency ordering for patch
+1 💚 mvninstall 3m 3s the patch passed
+1 💚 compile 3m 46s the patch passed
+1 💚 javac 3m 46s the patch passed
+1 💚 blanks 0m 0s The patch has no blanks issues.
+1 💚 checkstyle 0m 48s the patch passed
+1 💚 xmllint 0m 0s No new issues.
+1 💚 spotbugs 2m 21s the patch passed
+0 🆗 refguide 2m 4s patch has no errors when building the reference guide. See footer for rendered docs, which you should manually inspect.
+1 💚 hadoopcheck 11m 28s Patch does not cause any errors with Hadoop 3.3.6 3.4.0.
+1 💚 spotless 0m 44s patch has no errors when running spotless:check.
_ Other Tests _
+1 💚 asflicense 0m 17s The patch does not generate ASF License warnings.
45m 39s
Subsystem Report/Notes
Docker ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6422/4/artifact/yetus-general-check/output/Dockerfile
GITHUB PR #6422
Optional Tests dupname asflicense javac spotbugs checkstyle codespell detsecrets compile hadoopcheck hbaseanti spotless xmllint refguide
uname Linux c194f512c438 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality dev-support/hbase-personality.sh
git revision master / a401560
Default Java Eclipse Adoptium-17.0.11+9
refguide https://nightlies.apache.org/hbase/HBase-PreCommit-GitHub-PR/PR-6422/4/yetus-general-check/output/branch-site/book.html
refguide https://nightlies.apache.org/hbase/HBase-PreCommit-GitHub-PR/PR-6422/4/yetus-general-check/output/patch-site/book.html
Max. process+thread count 86 (vs. ulimit of 30000)
modules C: hbase-common hbase-server U: .
Console output https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6422/4/console
versions git=2.34.1 maven=3.9.8 spotbugs=4.7.3 xmllint=20913
Powered by Apache Yetus 0.15.0 https://yetus.apache.org

This message was automatically generated.

@Apache-HBase
Copy link

💔 -1 overall

Vote Subsystem Runtime Logfile Comment
+0 🆗 reexec 0m 43s Docker mode activated.
-0 ⚠️ yetus 0m 3s Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --author-ignore-list --blanks-eol-ignore-file --blanks-tabs-ignore-file --quick-hadoopcheck
_ Prechecks _
_ master Compile Tests _
+0 🆗 mvndep 0m 15s Maven dependency ordering for branch
+1 💚 mvninstall 2m 43s master passed
+1 💚 compile 1m 17s master passed
+1 💚 javadoc 0m 45s master passed
+1 💚 shadedjars 5m 0s branch has no errors when building our shaded downstream artifacts.
_ Patch Compile Tests _
+0 🆗 mvndep 0m 12s Maven dependency ordering for patch
+1 💚 mvninstall 3m 8s the patch passed
+1 💚 compile 1m 17s the patch passed
+1 💚 javac 1m 17s the patch passed
+1 💚 javadoc 0m 44s the patch passed
+1 💚 shadedjars 4m 51s patch has no errors when building our shaded downstream artifacts.
_ Other Tests _
+1 💚 unit 2m 6s hbase-common in the patch passed.
-1 ❌ unit 26m 42s /patch-unit-hbase-server.txt hbase-server in the patch failed.
52m 17s
Subsystem Report/Notes
Docker ClientAPI=1.47 ServerAPI=1.47 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6422/4/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile
GITHUB PR #6422
Optional Tests javac javadoc unit compile shadedjars
uname Linux 09253916f505 5.4.0-200-generic #220-Ubuntu SMP Fri Sep 27 13:19:16 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality dev-support/hbase-personality.sh
git revision master / a401560
Default Java Eclipse Adoptium-17.0.11+9
Test Results https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6422/4/testReport/
Max. process+thread count 2036 (vs. ulimit of 30000)
modules C: hbase-common hbase-server U: .
Console output https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6422/4/console
versions git=2.34.1 maven=3.9.8
Powered by Apache Yetus 0.15.0 https://yetus.apache.org

This message was automatically generated.

@ndimiduk ndimiduk merged commit c4fed77 into apache:master Dec 10, 2024
1 check failed
ndimiduk pushed a commit to ndimiduk/hbase that referenced this pull request Dec 10, 2024
ndimiduk pushed a commit to ndimiduk/hbase that referenced this pull request Dec 10, 2024
@ndimiduk
Copy link
Member

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?

ndimiduk pushed a commit to ndimiduk/hbase that referenced this pull request Dec 10, 2024
ndimiduk added a commit that referenced this pull request Dec 13, 2024
gvprathyusha6 pushed a commit to gvprathyusha6/hbase that referenced this pull request Dec 19, 2024
apurtell pushed a commit that referenced this pull request Jan 14, 2025
apurtell pushed a commit that referenced this pull request Jan 14, 2025
apurtell pushed a commit that referenced this pull request Jan 14, 2025
mokai87 pushed a commit to mokai87/hbase that referenced this pull request Aug 7, 2025
sanjeet006py pushed a commit to sanjeet006py/hbase that referenced this pull request Sep 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants