Copilot commented on code in PR #7514:
URL: https://github.com/apache/hbase/pull/7514#discussion_r2611598564
##########
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;
}
}
- return isConfigurationChange;
+ return false;
}
Review Comment:
The hasCoprocessorsConfigured method doesn't handle empty or whitespace-only
strings in the configuration values. When conf.getStrings(key) returns an array
containing empty strings, the method returns true even though no actual
coprocessors are configured. This is inconsistent with how the main
checkConfigurationChange method handles empty strings (using
Strings.isNullOrEmpty check on line 69). Consider filtering out
empty/whitespace-only strings before checking the array length.
##########
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);
Review Comment:
The checkConfigurationChange method doesn't check if coprocessors have been
disabled via the COPROCESSORS_ENABLED_CONF_KEY when coprocessorHost is not
null. This could lead to a scenario where coprocessors remain loaded even after
being disabled in the configuration. When coprocessors are enabled and
coprocessorHost exists, if a user sets hbase.coprocessor.enabled to false, the
method will still compare currently loaded coprocessors with the configured
classes and may incorrectly return false (no change) if the class names match,
even though coprocessors should be unloaded. Consider checking the enabled flag
in the main logic path as well.
--
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]
