HBASE-29526 Dynamic configuration not working for coprocessor #7506
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
Contributor
|
This problem only affects branch-2.x? |
Contributor
Author
There was a problem hiding this comment.
Pull request overview
This PR fixes HBASE-29526, addressing an issue where dynamic configuration changes for coprocessors were not being detected properly. The root cause was that the previous implementation compared old and new configuration objects, which failed when the configuration object was mutated in place (as happens during runtime configuration reloads).
Key changes:
- Replaced configuration object comparison with a comparison of currently loaded coprocessors versus newly configured coprocessors
- Added
getCoprocessorClassNames()method to CoprocessorHost to retrieve fully-qualified class names of loaded coprocessors - Updated tests to use the server's own configuration object, properly simulating dynamic configuration reload behavior
Reviewed changes
Copilot reviewed 6 out of 6 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/util/CoprocessorConfigurationUtil.java | Refactored checkConfigurationChange to compare loaded coprocessors against configuration values instead of comparing two configuration objects; added helper method to check if coprocessors are configured |
| hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java | Added getCoprocessorClassNames() method to retrieve fully-qualified class names of all loaded coprocessors for comparison |
| hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java | Updated to pass coprocessor host instead of old configuration to checkConfigurationChange |
| hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java | Updated to pass coprocessor host instead of old configuration to checkConfigurationChange |
| hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java | Updated to pass coprocessor host instead of old configuration; removed initializeCoprocessorHost helper method in favor of direct instantiation |
| hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerOnlineConfigChange.java | Updated tests to use the server's own configuration object (via getConfiguration()) instead of the test utility's configuration, correctly simulating dynamic configuration reload |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+93
to
+94
| if (coprocessors != null && coprocessors.length > 0) { | ||
| return true; |
There was a problem hiding this comment.
The hasCoprocessorsConfigured method should parse the className|priority|path format and check for empty class names, similar to how the main checkConfigurationChange method does at lines 65-72. Currently, this method returns true if the array length is greater than 0, even if all entries are empty or whitespace-only strings. This inconsistency could lead to incorrect detection of configuration changes when coprocessorHost is null.
The method should iterate through the coprocessor strings, parse each one using split("\\|"), extract the actual class name from token[0], trim it, and check if it's not null or empty before returning true.
Suggested change
| if (coprocessors != null && coprocessors.length > 0) { | |
| return true; | |
| if (coprocessors != null) { | |
| for (String coprocessor : coprocessors) { | |
| // Handle the className|priority|path format | |
| String[] tokens = coprocessor.split("\\|"); | |
| String actualClassName = tokens[0].trim(); | |
| if (!Strings.isNullOrEmpty(actualClassName)) { | |
| return true; | |
| } | |
| } |
Contributor
|
@Jaehui-Lee is right, it's not working for existing regions, let's have the review against master branch #7514 and then we can come back on here. |
…rocessor enabled check to top
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
taklwu
approved these changes
Dec 16, 2025
Contributor
taklwu
left a comment
taklwu
left a comment
There was a problem hiding this comment.
those failures should be unrelated
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://issues.apache.org/jira/browse/HBASE-29526