Copilot commented on code in PR #7506:
URL: https://github.com/apache/hbase/pull/7506#discussion_r2611598493
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/util/CoprocessorConfigurationUtil.java:
##########
@@ -32,19 +35,65 @@ public final class CoprocessorConfigurationUtil {
private CoprocessorConfigurationUtil() {
}
- public static boolean checkConfigurationChange(Configuration oldConfig,
Configuration newConfig,
- String... configurationKey) {
+ /**
+ * 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.
+ * @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 (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
+ 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) {
+ if (
+ !conf.getBoolean(CoprocessorHost.COPROCESSORS_ENABLED_CONF_KEY,
+ CoprocessorHost.DEFAULT_COPROCESSORS_ENABLED)
+ ) {
+ return false;
+ }
+
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;
Review 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.
```suggestion
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;
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
