HBASE-29526 Dynamic configuration not working for coprocessor (#7514)
Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org>
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 c201b29..d98d809 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
@@ -119,6 +119,19 @@
}
/**
+ * 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<String> getCoprocessorClassNames() {
+ Set<String> 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/ipc/RpcServer.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java
index 6dfb5bf..3fec4aa 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java
@@ -332,7 +332,7 @@
}
refreshSlowLogConfiguration(newConf);
if (
- CoprocessorConfigurationUtil.checkConfigurationChange(getConf(), newConf,
+ CoprocessorConfigurationUtil.checkConfigurationChange(this.cpHost, newConf,
CoprocessorHost.RPC_COPROCESSOR_CONF_KEY)
) {
LOG.info("Update the RPC coprocessor(s) because the configuration has changed");
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 79426bc..22d3ab6 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
@@ -1080,7 +1080,7 @@
if (!maintenanceMode) {
startupTaskGroup.addTask("Initializing master coprocessors");
setQuotasObserver(conf);
- initializeCoprocessorHost(conf);
+ this.cpHost = new MasterCoprocessorHost(this, conf);
} else {
// start an in process region server for carrying system regions
maintenanceRegionServer =
@@ -4417,11 +4417,11 @@
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);
}
}
@@ -4520,11 +4520,6 @@
}
}
- 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<byte[]> 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 9b7daee..5e050a5 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
@@ -8829,7 +8829,7 @@
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 cd49ceb..39fe632 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
@@ -3475,7 +3475,7 @@
// 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 93c88a8..18ded43 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,71 @@
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 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[] 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);
+ }
+ }
}
}
- return isConfigurationChange;
+
+ // 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[] coprocessors = conf.getStrings(key);
+ if (coprocessors != null && coprocessors.length > 0) {
+ return true;
+ }
+ }
+ 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 b4bbf42..00890e8 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
@@ -267,8 +267,10 @@
@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()));
}
@@ -276,9 +278,11 @@
@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()));
}
