HBASE-29526 Dynamic configuration not working for coprocessor

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

@@ -1025,7 +1025,7 @@ private void finishActiveMasterInitialization()
     if (!maintenanceMode) {
       startupTaskGroup.addTask("Initializing master coprocessors");
       setQuotasObserver(conf);
-      initializeCoprocessorHost(conf);
+      this.cpHost = new MasterCoprocessorHost(this, conf);
     }
 
     // Checking if meta needs initializing.
@@ -4264,11 +4264,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);
     }
   }
 
@@ -4286,9 +4286,4 @@ 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);
-  }
-
 }

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

@@ -8418,7 +8418,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

@@ -3850,7 +3850,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,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)
+    ) {
+      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;
   }
 }

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

@@ -7745,26 +7745,25 @@ public void testRegionOnCoprocessorsChange() throws IOException {
     assertNull(region.getCoprocessorHost());
 
     // set and verify the system coprocessors for region and user region
-    Configuration newConf = new Configuration(conf);
-    newConf.set(CoprocessorHost.REGION_COPROCESSOR_CONF_KEY, MetaTableMetrics.class.getName());
-    newConf.set(CoprocessorHost.USER_REGION_COPROCESSOR_CONF_KEY,
+    conf.set(CoprocessorHost.REGION_COPROCESSOR_CONF_KEY, MetaTableMetrics.class.getName());
+    conf.set(CoprocessorHost.USER_REGION_COPROCESSOR_CONF_KEY,
       NoOpRegionCoprocessor.class.getName());
     // trigger configuration change
-    region.onConfigurationChange(newConf);
-    assertTrue(region.getCoprocessorHost() != null);
+    region.onConfigurationChange(conf);
+    assertNotNull(region.getCoprocessorHost());
     Set<String> coprocessors = region.getCoprocessorHost().getCoprocessors();
-    assertTrue(coprocessors.size() == 2);
+    assertEquals(2, coprocessors.size());
     assertTrue(region.getCoprocessorHost().getCoprocessors()
       .contains(MetaTableMetrics.class.getSimpleName()));
     assertTrue(region.getCoprocessorHost().getCoprocessors()
       .contains(NoOpRegionCoprocessor.class.getSimpleName()));
 
     // remove region coprocessor and keep only user region coprocessor
-    newConf.unset(CoprocessorHost.REGION_COPROCESSOR_CONF_KEY);
-    region.onConfigurationChange(newConf);
-    assertTrue(region.getCoprocessorHost() != null);
+    conf.unset(CoprocessorHost.REGION_COPROCESSOR_CONF_KEY);
+    region.onConfigurationChange(conf);
+    assertNotNull(region.getCoprocessorHost());
     coprocessors = region.getCoprocessorHost().getCoprocessors();
-    assertTrue(coprocessors.size() == 1);
+    assertEquals(1, coprocessors.size());
     assertTrue(region.getCoprocessorHost().getCoprocessors()
       .contains(NoOpRegionCoprocessor.class.getSimpleName()));
   }

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()));
   }