From 11823ae6af8055cec3f297147a0b186603bb2aeb Mon Sep 17 00:00:00 2001 From: Jaehui-Lee Date: Thu, 14 Aug 2025 10:00:31 +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 | 70 ++++++++++++++++--- .../hbase/regionserver/TestHRegion.java | 19 +++-- .../TestRegionServerOnlineConfigChange.java | 13 ++-- 7 files changed, 96 insertions(+), 34 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..3aec454bef43 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 f9e75a0438fb..074bc319a4c6 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 @@ -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); - } - } 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 ca0e7db716dd..8d56b730e91c 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 @@ -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) ) { 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 afa47425380c..512e2cb76f2d 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 @@ -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"); 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..dae33b044d42 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 @@ -19,9 +19,14 @@ import org.apache.commons.lang3.StringUtils; 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 java.util.HashSet; +import java.util.Set; + +import org.apache.hbase.thirdparty.com.google.common.base.Strings; /** * Helper class for coprocessor host when configuration changes. @@ -32,19 +37,64 @@ 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/TestHRegion.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java index c26dcdd6fe7a..f821a1536a73 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java +++ b/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 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())); } 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..d9db026e6ff3 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 @@ -34,6 +34,7 @@ import org.apache.hadoop.hbase.client.Table; import org.apache.hadoop.hbase.client.TableDescriptorBuilder; import org.apache.hadoop.hbase.coprocessor.CoprocessorHost; +import org.apache.hadoop.hbase.coprocessor.MetaTableMetrics; import org.apache.hadoop.hbase.master.HMaster; import org.apache.hadoop.hbase.regionserver.compactions.CompactionConfiguration; import org.apache.hadoop.hbase.testclassification.MediumTests; @@ -230,8 +231,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 +242,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 c0927c437427742bab9452ba4b721d4227dc0f21 Mon Sep 17 00:00:00 2001 From: Jaehui-Lee Date: Thu, 14 Aug 2025 11:48:23 +0900 Subject: [PATCH 2/2] Run spotless:apply --- .../hbase/coprocessor/CoprocessorHost.java | 6 +++--- .../util/CoprocessorConfigurationUtil.java | 17 ++++++++--------- .../TestRegionServerOnlineConfigChange.java | 1 - 3 files changed, 11 insertions(+), 13 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 3aec454bef43..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 @@ -116,9 +116,9 @@ public Set getCoprocessors() { } /** - * 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. + * 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<>(); 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 dae33b044d42..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,15 +17,13 @@ */ 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 java.util.HashSet; -import java.util.Set; - import org.apache.hbase.thirdparty.com.google.common.base.Strings; /** @@ -41,9 +39,8 @@ 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. - * - * @param coprocessorHost the coprocessor host to check current loaded coprocessors (can be null) - * @param conf the configuration to check + * @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 */ @@ -84,8 +81,10 @@ 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)) { + if ( + !conf.getBoolean(CoprocessorHost.COPROCESSORS_ENABLED_CONF_KEY, + CoprocessorHost.DEFAULT_COPROCESSORS_ENABLED) + ) { 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 d9db026e6ff3..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 @@ -34,7 +34,6 @@ import org.apache.hadoop.hbase.client.Table; import org.apache.hadoop.hbase.client.TableDescriptorBuilder; import org.apache.hadoop.hbase.coprocessor.CoprocessorHost; -import org.apache.hadoop.hbase.coprocessor.MetaTableMetrics; import org.apache.hadoop.hbase.master.HMaster; import org.apache.hadoop.hbase.regionserver.compactions.CompactionConfiguration; import org.apache.hadoop.hbase.testclassification.MediumTests;