Copilot commented on code in PR #7544:
URL: https://github.com/apache/hbase/pull/7544#discussion_r2621711759
##########
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();
+
Review 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.
```suggestion
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();
}
```
##########
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 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.
##########
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");
Review 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".
```suggestion
Preconditions.checkArgument(configurationKey != null, "Configuration
Key(s) must be provided");
Preconditions.checkArgument(configurationKey.length > 0, "At least one
configuration key must be provided");
```
##########
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);
+ }
Review 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.
--
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]
