Copilot commented on code in PR #7543:
URL: https://github.com/apache/hbase/pull/7543#discussion_r2621710519
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/util/CoprocessorConfigurationUtil.java:
##########
@@ -32,19 +35,71 @@ 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.
+ * <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;
}
}
Review 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
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/util/CoprocessorConfigurationUtil.java:
##########
@@ -32,19 +35,71 @@ 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.
+ * <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;
}
Review 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.
--
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]
