From bcefa41b00d7f1ce3ae0420ee87db7ea5efd8606 Mon Sep 17 00:00:00 2001 From: Jaehui-Lee Date: Wed, 3 Dec 2025 21:39:23 +0900 Subject: [PATCH 1/2] HBASE-29526 Dynamic configuration not working for coprocessor --- .../hbase/coprocessor/CoprocessorHost.java | 13 ++++ .../apache/hadoop/hbase/master/HMaster.java | 11 +-- .../hadoop/hbase/regionserver/HRegion.java | 2 +- .../hbase/regionserver/HRegionServer.java | 2 +- .../util/CoprocessorConfigurationUtil.java | 71 ++++++++++++++++--- .../TestRegionServerOnlineConfigChange.java | 12 ++-- 6 files changed, 86 insertions(+), 25 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java index c1ba9e274adb..c8199058c362 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java @@ -115,6 +115,19 @@ public Set getCoprocessors() { return returnValue; } + /** + * Get the full class names of all loaded coprocessors. This method returns the complete class + * names including package information, which is useful for precise coprocessor identification and + * comparison. + */ + public Set getCoprocessorClassNames() { + Set returnValue = new TreeSet<>(); + for (E e : coprocEnvironments) { + returnValue.add(e.getInstance().getClass().getName()); + } + return returnValue; + } + /** * Load system coprocessors once only. Read the class names from configuration. Called by * constructor. diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java index 9e6e929eb10d..b815be3613eb 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java @@ -1054,7 +1054,7 @@ private void finishActiveMasterInitialization() throws IOException, InterruptedE if (!maintenanceMode) { startupTaskGroup.addTask("Initializing master coprocessors"); setQuotasObserver(conf); - initializeCoprocessorHost(conf); + this.cpHost = new MasterCoprocessorHost(this, conf); } // Checking if meta needs initializing. @@ -4380,11 +4380,11 @@ public void onConfigurationChange(Configuration newConf) { setQuotasObserver(newConf); // update region server coprocessor if the configuration has changed. if ( - CoprocessorConfigurationUtil.checkConfigurationChange(getConfiguration(), newConf, + CoprocessorConfigurationUtil.checkConfigurationChange(this.cpHost, newConf, CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY) && !maintenanceMode ) { LOG.info("Update the master coprocessor(s) because the configuration has changed"); - initializeCoprocessorHost(newConf); + this.cpHost = new MasterCoprocessorHost(this, newConf); } } @@ -4396,11 +4396,6 @@ private void setQuotasObserver(Configuration conf) { } } - private void initializeCoprocessorHost(Configuration conf) { - // initialize master side coprocessors before we start handling requests - this.cpHost = new MasterCoprocessorHost(this, conf); - } - @Override public long flushTable(TableName tableName, List columnFamilies, long nonceGroup, long nonce) throws IOException { diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java index f75e8f5ac5e4..9e8c5c5db524 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java @@ -8528,7 +8528,7 @@ public void onConfigurationChange(Configuration conf) { this.storeHotnessProtector.update(conf); // update coprocessorHost if the configuration has changed. if ( - CoprocessorConfigurationUtil.checkConfigurationChange(getReadOnlyConfiguration(), conf, + CoprocessorConfigurationUtil.checkConfigurationChange(this.coprocessorHost, conf, CoprocessorHost.REGION_COPROCESSOR_CONF_KEY, CoprocessorHost.USER_REGION_COPROCESSOR_CONF_KEY) ) { diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java index 4085315ea882..cf91f744bf7b 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java @@ -4010,7 +4010,7 @@ public void onConfigurationChange(Configuration newConf) { // update region server coprocessor if the configuration has changed. if ( - CoprocessorConfigurationUtil.checkConfigurationChange(getConfiguration(), newConf, + CoprocessorConfigurationUtil.checkConfigurationChange(this.rsHost, newConf, CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY) ) { LOG.info("Update region server coprocessors because the configuration has changed"); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/util/CoprocessorConfigurationUtil.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/util/CoprocessorConfigurationUtil.java index 93c88a897717..6fb66489c211 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/util/CoprocessorConfigurationUtil.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/util/CoprocessorConfigurationUtil.java @@ -17,11 +17,14 @@ */ package org.apache.hadoop.hbase.util; -import org.apache.commons.lang3.StringUtils; +import java.util.HashSet; +import java.util.Set; import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hbase.coprocessor.CoprocessorHost; import org.apache.yetus.audience.InterfaceAudience; import org.apache.hbase.thirdparty.com.google.common.base.Preconditions; +import org.apache.hbase.thirdparty.com.google.common.base.Strings; /** * Helper class for coprocessor host when configuration changes. @@ -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 currentlyLoaded = coprocessorHost.getCoprocessorClassNames(); + + // Get coprocessor class names from configuration + Set 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; } } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerOnlineConfigChange.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerOnlineConfigChange.java index 961c08f8a16a..de6f926f71a1 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerOnlineConfigChange.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerOnlineConfigChange.java @@ -230,8 +230,10 @@ public void testStoreConfigurationOnlineChange() { @Test public void testCoprocessorConfigurationOnlineChange() { assertNull(rs1.getRegionServerCoprocessorHost().findCoprocessor(JMXListener.class.getName())); - conf.set(CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY, JMXListener.class.getName()); - rs1.getConfigurationManager().notifyAllObservers(conf); + // Update configuration directly to simulate dynamic configuration reload + Configuration rsConf = rs1.getConfiguration(); + rsConf.set(CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY, JMXListener.class.getName()); + rs1.getConfigurationManager().notifyAllObservers(rsConf); assertNotNull( rs1.getRegionServerCoprocessorHost().findCoprocessor(JMXListener.class.getName())); } @@ -239,9 +241,11 @@ public void testCoprocessorConfigurationOnlineChange() { @Test public void testCoprocessorConfigurationOnlineChangeOnMaster() { assertNull(hMaster.getMasterCoprocessorHost().findCoprocessor(JMXListener.class.getName())); - conf.set(CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY, JMXListener.class.getName()); + // Update configuration directly to simulate dynamic configuration reload + Configuration masterConf = hMaster.getConfiguration(); + masterConf.set(CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY, JMXListener.class.getName()); assertFalse(hMaster.isInMaintenanceMode()); - hMaster.getConfigurationManager().notifyAllObservers(conf); + hMaster.getConfigurationManager().notifyAllObservers(masterConf); assertNotNull(hMaster.getMasterCoprocessorHost().findCoprocessor(JMXListener.class.getName())); } From 8de18001def9b9d23783cd831683974de25bf240 Mon Sep 17 00:00:00 2001 From: Jaehui-Lee Date: Fri, 12 Dec 2025 14:45:45 +0900 Subject: [PATCH 2/2] Add documentation for priority/path detection limitation and move coprocessor enabled check to top --- .../util/CoprocessorConfigurationUtil.java | 20 ++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/util/CoprocessorConfigurationUtil.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/util/CoprocessorConfigurationUtil.java index 6fb66489c211..18ded4319adf 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/util/CoprocessorConfigurationUtil.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/util/CoprocessorConfigurationUtil.java @@ -39,6 +39,11 @@ private CoprocessorConfigurationUtil() { * 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. + *

+ * Note: This method only detects changes in the set of coprocessor class names. It does + * not 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 @@ -49,6 +54,13 @@ public static boolean checkConfigurationChange(CoprocessorHost coprocessor Preconditions.checkArgument(configurationKey != null, "Configuration Key(s) must be provided"); 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); @@ -58,6 +70,7 @@ public static boolean checkConfigurationChange(CoprocessorHost coprocessor Set currentlyLoaded = coprocessorHost.getCoprocessorClassNames(); // Get coprocessor class names from configuration + // Only class names are compared; priority and path changes are not detected Set configuredClasses = new HashSet<>(); for (String key : configurationKey) { String[] classes = conf.getStrings(key); @@ -81,13 +94,6 @@ public static boolean checkConfigurationChange(CoprocessorHost coprocessor * 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[] coprocessors = conf.getStrings(key); if (coprocessors != null && coprocessors.length > 0) {