HBASE-29526 Dynamic configuration not working for coprocessor (#7506)

Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org>

Author: Jaehui-Lee

hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java

@@ -115,6 +115,19 @@ public Set<String> 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<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.

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<byte[]> columnFamilies, long nonceGroup,
     long nonce) throws IOException {

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java

@@ -8541,7 +8541,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)
     ) {

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");

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 @@ 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;
   }
 }

hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerOnlineConfigChange.java

@@ -230,18 +230,22 @@ 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()));
   }
 
   @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()));
   }