Skip to content

HBASE-29249 Allow for BlockCache implementations to define dynamic properties - #6897

Merged
wchevreuil merged 6 commits into
apache:masterfrom
wchevreuil:HBASE-29249
Apr 25, 2025
Merged

HBASE-29249 Allow for BlockCache implementations to define dynamic properties#6897
wchevreuil merged 6 commits into
apache:masterfrom
wchevreuil:HBASE-29249

Conversation

@wchevreuil

@wchevreuilwchevreuil commented Apr 9, 2025

Copy link
Copy Markdown
Contributor

No description provided.

@wchevreuil
HBASE-29249 Allow for BlockCache implementations to define dynamic pr…
77209a7
…operties
Change-Id: I3681c3b1a50196a70ece04fde08da06448d16353
@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
Apache9
Apache9 reviewed Apr 10, 2025
View reviewed changes
Comment threadhbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/BlockCache.java Outdated
* notifications of dynamic configuration changes. The default is a noop.
* @param config the new configuration to be updated.
*/
default void refreshConfiguration(Configuration config) {

@Apache9Apache9Apr 10, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not just implement ConfigurationObserver?

@wchevreuilwchevreuilApr 10, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I just thought to reuse CacheConfig as the observer for all cache related configuration changes notifications. Sounds less convoluted to have just one observer, but no hard feelings about it. I can change if you prefer to have BlockCache implementing ConfigurationObserver.

@Apache9Apache9Apr 10, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We can make CacheConfig implement PropagatingConfigurationObserver, so it can register children ConfigurationObservers to ConfigurationManager.

@wchevreuilwchevreuilApr 10, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ok, have pushed a new commit with these suggestions.

@Apache-HBase

This comment has been minimized.

Sign in to view
@wchevreuil
Addressing review comments
7f14a1a
Change-Id: I0aac16ed7d685ac533fe9d0325747be03adc9140
@wchevreuil
temp
378dc2f
Change-Id: I72430e198d729500cd314ea1ec59d05edb8f415a
@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@wchevreuil
trying to address flakeyness (not related to the original change)
71cead7
Change-Id: I12afe38cd3df15974a44101accd74614edec2a01
@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
Apache9
Apache9 reviewed Apr 12, 2025
View reviewed changes
Comment threadhbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java
Comment threadhbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java
Comment threadhbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java

@Override
public void onConfigurationChange(Configuration config) {
this.acceptableFactor = conf.getFloat(ACCEPT_FACTOR_CONFIG_NAME, DEFAULT_ACCEPT_FACTOR);

@Apache9Apache9Apr 12, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Haven't touch this area for a long time, all these configs can be loaded dynamically? IIRC, BucketCache will pre alloc all the memory chunks when initializing, configuration refreshing can not trigger the allocation again.

@wchevreuilwchevreuilApr 22, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes. We can't make any of the bucket allocation related properties, such as bucket sizes and cache size dynamic as those would require a reset of the cache.

The properties being updated here are not related to the cache size, nor how the cache spaces are allocated, so it won't cause a cache reset.

@taklwutaklwuApr 24, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: can we add a comment of javadoc on what properties could not be dynamic config? basically we changed ConfigurationObserver to be part of the dynamic config.

@wchevreuil
spotless
25beeca
Change-Id: Ia7d67f5593aaaefe591e438c00036c2846dc7f8f
@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

Apache-HBase commented Apr 23, 2025

Copy link
Copy Markdown

🎊 +1 overall

VoteSubsystemRuntimeLogfileComment
+0 🆗reexec0m 28sDocker mode activated.
_ Prechecks _
+1 💚dupname0m 0sNo case conflicting files found.
+0 🆗codespell0m 0scodespell was not available.
+0 🆗detsecrets0m 0sdetect-secrets was not available.
+1 💚@author0m 0sThe patch does not contain any @author tags.
+1 💚hbaseanti0m 0sPatch does not have any anti-patterns.
_ master Compile Tests _
+1 💚mvninstall4m 13smaster passed
+1 💚compile3m 34smaster passed
+1 💚checkstyle0m 43smaster passed
+1 💚spotbugs1m 54smaster passed
+1 💚spotless0m 54sbranch has no errors when running spotless:check.
_ Patch Compile Tests _
+1 💚mvninstall3m 40sthe patch passed
+1 💚compile3m 31sthe patch passed
+1 💚javac3m 31sthe patch passed
+1 💚blanks0m 0sThe patch has no blanks issues.
+1 💚checkstyle0m 41sthe patch passed
+1 💚spotbugs2m 0sthe patch passed
+1 💚hadoopcheck12m 51sPatch does not cause any errors with Hadoop 3.3.6 3.4.0.
+1 💚spotless0m 52spatch has no errors when running spotless:check.
_ Other Tests _
+1 💚asflicense0m 12sThe patch does not generate ASF License warnings.
43m 24s
SubsystemReport/Notes
DockerClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6897/6/artifact/yetus-general-check/output/Dockerfile
GITHUB PR#6897
Optional Testsdupname asflicense javac spotbugs checkstyle codespell detsecrets compile hadoopcheck hbaseanti spotless
unameLinux b9bd6a5d5d65 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 toolmaven
Personalitydev-support/hbase-personality.sh
git revisionmaster / 25beeca
Default JavaEclipse Adoptium-17.0.11+9
Max. process+thread count84 (vs. ulimit of 30000)
modulesC: hbase-server U: hbase-server
Console outputhttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6897/6/console
versionsgit=2.34.1 maven=3.9.8 spotbugs=4.7.3
Powered byApache Yetus 0.15.0 https://yetus.apache.org

This message was automatically generated.

@Apache-HBase

Apache-HBase commented Apr 23, 2025

Copy link
Copy Markdown

💔 -1 overall

VoteSubsystemRuntimeLogfileComment
+0 🆗reexec0m 31sDocker mode activated.
-0 ⚠️yetus0m 2sUnprocessed 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 _
+1 💚mvninstall5m 9smaster passed
+1 💚compile1m 27smaster passed
+1 💚javadoc0m 39smaster passed
+1 💚shadedjars6m 58sbranch has no errors when building our shaded downstream artifacts.
_ Patch Compile Tests _
+1 💚mvninstall3m 42sthe patch passed
+1 💚compile1m 12sthe patch passed
+1 💚javac1m 12sthe patch passed
+1 💚javadoc0m 32sthe patch passed
+1 💚shadedjars6m 13spatch has no errors when building our shaded downstream artifacts.
_ Other Tests _
-1 ❌unit243m 32s/patch-unit-hbase-server.txthbase-server in the patch failed.
274m 37s
SubsystemReport/Notes
DockerClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6897/6/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile
GITHUB PR#6897
Optional Testsjavac javadoc unit compile shadedjars
unameLinux a6b516ff0430 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 toolmaven
Personalitydev-support/hbase-personality.sh
git revisionmaster / 25beeca
Default JavaEclipse Adoptium-17.0.11+9
Test Resultshttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6897/6/testReport/
Max. process+thread count4444 (vs. ulimit of 30000)
modulesC: hbase-server U: hbase-server
Console outputhttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6897/6/console
versionsgit=2.34.1 maven=3.9.8
Powered byApache Yetus 0.15.0 https://yetus.apache.org

This message was automatically generated.

taklwu
taklwu approved these changes Apr 24, 2025
View reviewed changes

@taklwutaklwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1 , add a minor comment about putting a comment what could be or could not be dynamic configuration in the code (or just say be cautions about this function.)

@wchevreuil
added javadoc comments
660b17b
Change-Id: Id6390798d91d6f557caa0ed0007eda817c4ac16c
@wchevreuil
wchevreuil merged commit ae40069 into apache:masterApr 25, 2025
wchevreuil added a commit that referenced this pull request Apr 25, 2025
@wchevreuil
HBASE-29249Allow for BlockCache implementations to define dynamic pr…
cfb1885
…operties (#6897)
Signed-off-by: Duo Zhang <zhangduo@apache.org>
Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org>
wchevreuil added a commit to wchevreuil/hbase that referenced this pull request Apr 25, 2025
@wchevreuil
HBASE-29249 Allow for BlockCache implementations to define dynamic pr…
aef2736
…operties (apache#6897)
Signed-off-by: Duo Zhang <zhangduo@apache.org>
Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@taklwutaklwutaklwu approved these changes
@Apache9Apache9Apache9 approved these changes
@chrajeshbabuchrajeshbabuAwaiting requested review from chrajeshbabu
@ankitsinghalankitsinghalAwaiting requested review from ankitsinghal
@petersomogyipetersomogyiAwaiting requested review from petersomogyi

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@wchevreuil@Apache-HBase@taklwu@Apache9