HBASE-29526 Dynamic configuration not working for coprocessor #7544
Conversation
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
taklwu
left a comment
taklwu
left a comment
There was a problem hiding this comment.
docker yetus failure is unrelated as reported in https://issues.apache.org/jira/browse/INFRA-27492, I will keep the PR open for another day and see if they can come back and look at the logs
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where dynamic configuration changes for coprocessors were not being properly detected and applied. The root cause was that the configuration change detection was comparing two Configuration objects instead of comparing the currently loaded coprocessors with the new configuration values.
Key Changes:
- Refactored
CoprocessorConfigurationUtil.checkConfigurationChange()to compare current loaded coprocessors against new configuration instead of comparing two configuration objects - Added
getCoprocessorClassNames()method to CoprocessorHost to retrieve currently loaded coprocessor class names - Updated all call sites to pass the CoprocessorHost instance instead of the old Configuration object
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| hbase-server/src/main/java/org/apache/hadoop/hbase/util/CoprocessorConfigurationUtil.java | Rewrote checkConfigurationChange() to compare current state vs new config; added hasCoprocessorsConfigured() helper method |
| hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java | Added getCoprocessorClassNames() method to expose currently loaded coprocessor class names |
| hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java | Updated to pass rsHost instance instead of old configuration to checkConfigurationChange() |
| hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java | Updated to pass coprocessorHost instance instead of old configuration to checkConfigurationChange() |
| hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java | Updated to pass cpHost instance instead of old configuration; inlined initializeCoprocessorHost() method |
| hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java | Updated to pass cpHost instance instead of old configuration to checkConfigurationChange() |
| hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerOnlineConfigChange.java | Updated tests to modify the actual server configuration object to properly test dynamic configuration reload |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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 should validate that the coprocessor strings contain actual class names, not just check if the array is non-empty. If the configuration contains empty strings or only whitespace, this method will incorrectly return true even though no valid coprocessors are configured. This is inconsistent with the logic in checkConfigurationChange (lines 78-84) which properly filters empty class names. The method should parse the className|priority|path format and check if the actual class name is non-empty after trimming.
| 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(); | ||
|
|
There was a problem hiding this comment.
When coprocessors are disabled in the new configuration (COPROCESSORS_ENABLED_CONF_KEY is false), this method returns false indicating no configuration change. However, if coprocessors were previously enabled and loaded, this prevents the coprocessor host from being recreated to unload those coprocessors. The method should return true if there's a mismatch between the current loaded state and the desired state, even when coprocessors are being disabled. Consider checking if currentlyLoaded is non-empty when coprocessors are disabled - that would indicate a change requiring coprocessor unloading.
| 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(); | |
| if (coprocessorHost == null) { | |
| // If no coprocessor host exists, check if any coprocessors are now configured | |
| if (!conf.getBoolean(CoprocessorHost.COPROCESSORS_ENABLED_CONF_KEY, | |
| CoprocessorHost.DEFAULT_COPROCESSORS_ENABLED)) { | |
| // Coprocessors are disabled and none are loaded, so no change | |
| return false; | |
| } | |
| return hasCoprocessorsConfigured(conf, configurationKey); | |
| } | |
| // Get currently loaded coprocessor class names | |
| Set<String> currentlyLoaded = coprocessorHost.getCoprocessorClassNames(); | |
| if (!conf.getBoolean(CoprocessorHost.COPROCESSORS_ENABLED_CONF_KEY, | |
| CoprocessorHost.DEFAULT_COPROCESSORS_ENABLED)) { | |
| // Coprocessors are now disabled, but if any are currently loaded, we need to unload them | |
| return !currentlyLoaded.isEmpty(); | |
| } |
| */ | ||
| public static boolean checkConfigurationChange(CoprocessorHost<?, ?> coprocessorHost, | ||
| Configuration conf, String... configurationKey) { | ||
| Preconditions.checkArgument(configurationKey != null, "Configuration Key(s) must be provided"); |
There was a problem hiding this comment.
The precondition check verifies that configurationKey is not null, but doesn't check if the array is empty. If an empty array is passed, the method will always return false (no change) even if coprocessors are currently loaded and should be unloaded. Consider adding a check for array length or documenting that an empty array is allowed and means "no configuration keys to check".
| Preconditions.checkArgument(configurationKey != null, "Configuration Key(s) must be provided"); | |
| Preconditions.checkArgument(configurationKey != null, "Configuration Key(s) must be provided"); | |
| Preconditions.checkArgument(configurationKey.length > 0, "At least one configuration key must be provided"); |
| 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); | ||
| } |
There was a problem hiding this comment.
The new checkConfigurationChange method lacks unit test coverage. While integration tests exist in TestRegionServerOnlineConfigChange, they don't cover important edge cases such as: (1) disabling coprocessors when some are loaded, (2) the null coprocessorHost path that calls hasCoprocessorsConfigured, (3) parsing of className|priority|path format, (4) removing coprocessors from configuration, and (5) empty or whitespace-only class names. Consider adding unit tests for CoprocessorConfigurationUtil to ensure these scenarios are properly handled.
3 participants
https://issues.apache.org/jira/browse/HBASE-29526
Backport to branch-3