HBASE-29526 Dynamic configuration not working for coprocessor #7543
+92
−25
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.
taklwu
approved these changes
Dec 16, 2025
Contributor
taklwu
left a comment
taklwu
left a comment
There was a problem hiding this comment.
docker yetus failure has been reported, unit test failures are unrelated
There was a problem hiding this comment.
Pull request overview
This PR fixes HBASE-29526, which addresses an issue where dynamic configuration updates for coprocessors were not working correctly. The root cause was that the configuration change detection was comparing two configuration objects instead of comparing currently loaded coprocessors with the new configuration values.
Key Changes:
- Modified
CoprocessorConfigurationUtil.checkConfigurationChange()to compare currently loaded coprocessors with configuration values instead of comparing two configuration objects - Added
getCoprocessorClassNames()method toCoprocessorHostto retrieve the full class names of loaded coprocessors - Updated test cases to modify the server's configuration object directly rather than using a separate configuration instance, which simulates the actual dynamic configuration reload behavior
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
CoprocessorConfigurationUtil.java |
Refactored configuration change detection to compare currently loaded coprocessors against new configuration values, added helper method for checking configured coprocessors |
CoprocessorHost.java |
Added getCoprocessorClassNames() method to return full class names of loaded coprocessors for comparison |
HRegionServer.java |
Updated to pass coprocessor host reference instead of old configuration to change detection method |
HRegion.java |
Updated to pass coprocessor host reference instead of old configuration to change detection method |
HMaster.java |
Updated to pass coprocessor host reference, removed redundant initializeCoprocessorHost() wrapper method |
TestRegionServerOnlineConfigChange.java |
Modified tests to update configuration directly on server instances to properly simulate dynamic configuration reload |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+96
to
104
| private static boolean hasCoprocessorsConfigured(Configuration conf, String... configurationKey) { | ||
| for (String key : configurationKey) { | ||
| String oldValue = oldConfig.get(key); | ||
| String newValue = newConfig.get(key); | ||
| // check if the coprocessor key has any difference | ||
| if (!StringUtils.equalsIgnoreCase(oldValue, newValue)) { | ||
| isConfigurationChange = true; | ||
| break; | ||
| String[] coprocessors = conf.getStrings(key); | ||
| if (coprocessors != null && coprocessors.length > 0) { | ||
| return true; | ||
| } | ||
| } | ||
| return isConfigurationChange; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The hasCoprocessorsConfigured method does not properly validate coprocessor entries. It returns true if the array is non-empty, but doesn't check if the entries contain actual coprocessor class names (they could be empty strings or whitespace). This could cause the method to return true even when no valid coprocessors are configured.
Consider checking if any of the entries actually contain non-empty class names after parsing, similar to how configuredClasses is built in checkConfigurationChange.
Comment on lines
+38
to
105
| /** | ||
| * Check configuration change by comparing current loaded coprocessors with configuration values. | ||
| * This method is useful when the configuration object has been updated but we need to determine | ||
| * if coprocessor configuration has actually changed compared to what's currently loaded. | ||
| * <p> | ||
| * <b>Note:</b> This method only detects changes in the set of coprocessor class names. It does | ||
| * <b>not</b> detect changes to priority or path for coprocessors that are already loaded with the | ||
| * same class name. If you need to update the priority or path of an existing coprocessor, you | ||
| * must restart the region/regionserver/master. | ||
| * @param coprocessorHost the coprocessor host to check current loaded coprocessors (can be null) | ||
| * @param conf the configuration to check | ||
| * @param configurationKey the configuration keys to check | ||
| * @return true if configuration has changed, false otherwise | ||
| */ | ||
| public static boolean checkConfigurationChange(CoprocessorHost<?, ?> coprocessorHost, | ||
| Configuration conf, String... configurationKey) { | ||
| Preconditions.checkArgument(configurationKey != null, "Configuration Key(s) must be provided"); | ||
| boolean isConfigurationChange = false; | ||
| Preconditions.checkArgument(conf != null, "Configuration must be provided"); | ||
|
|
||
| if ( | ||
| !conf.getBoolean(CoprocessorHost.COPROCESSORS_ENABLED_CONF_KEY, | ||
| CoprocessorHost.DEFAULT_COPROCESSORS_ENABLED) | ||
| ) { | ||
| return false; | ||
| } | ||
|
|
||
| if (coprocessorHost == null) { | ||
| // If no coprocessor host exists, check if any coprocessors are now configured | ||
| return hasCoprocessorsConfigured(conf, configurationKey); | ||
| } | ||
|
|
||
| // Get currently loaded coprocessor class names | ||
| Set<String> currentlyLoaded = coprocessorHost.getCoprocessorClassNames(); | ||
|
|
||
| // Get coprocessor class names from configuration | ||
| // Only class names are compared; priority and path changes are not detected | ||
| Set<String> configuredClasses = new HashSet<>(); | ||
| for (String key : configurationKey) { | ||
| String[] classes = conf.getStrings(key); | ||
| if (classes != null) { | ||
| for (String className : classes) { | ||
| // Handle the className|priority|path format | ||
| String[] classNameToken = className.split("\\|"); | ||
| String actualClassName = classNameToken[0].trim(); | ||
| if (!Strings.isNullOrEmpty(actualClassName)) { | ||
| configuredClasses.add(actualClassName); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Compare the two sets | ||
| return !currentlyLoaded.equals(configuredClasses); | ||
| } | ||
|
|
||
| /** | ||
| * Helper method to check if there are any coprocessors configured. | ||
| */ | ||
| private static boolean hasCoprocessorsConfigured(Configuration conf, String... configurationKey) { | ||
| for (String key : configurationKey) { | ||
| String oldValue = oldConfig.get(key); | ||
| String newValue = newConfig.get(key); | ||
| // check if the coprocessor key has any difference | ||
| if (!StringUtils.equalsIgnoreCase(oldValue, newValue)) { | ||
| isConfigurationChange = true; | ||
| break; | ||
| String[] coprocessors = conf.getStrings(key); | ||
| if (coprocessors != null && coprocessors.length > 0) { | ||
| return true; | ||
| } | ||
| } | ||
| return isConfigurationChange; | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
The significantly modified checkConfigurationChange method and the new hasCoprocessorsConfigured helper method lack direct unit test coverage. Given that this utility has comprehensive logic for comparing coprocessor configurations and handles edge cases like parsing className|priority|path format, these methods should have dedicated unit tests to verify correctness.
Consider adding a TestCoprocessorConfigurationUtil test class to cover:
- Checking configuration changes when coprocessor host is null
- Checking configuration changes with various coprocessor class name formats
- Handling className|priority|path format parsing
- Verifying the behavior when coprocessors are enabled/disabled
- Testing the hasCoprocessorsConfigured helper with various inputs
|
🎊 +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. |
Contributor
|
TestProcDispatcher.testRetryLimitOnConnClosedErrors timeout , so it's unrelated, merging |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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
Backport to branch-2.6