taklwu commented on code in PR #7514:
URL: https://github.com/apache/hbase/pull/7514#discussion_r2611827753
##########
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)
Review Comment:
maybe we should move this `COPROCESSORS_ENABLED_CONF_KEY` check to the top
of `checkConfigurationChange`
##########
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);
+ }
+ }
+ }
+ }
Review Comment:
this made me rethink about the configuration change detection cannot handle
`path` reload, and even if we can detect changes for classname and priority.
So, can you update a note/comment in the function
`checkConfigurationChange`, that priority and path changes are not supported
with the same classname once it's loaded?
--
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]
