From cbfa841ec492d3a81d1c6f273d8c9ab1fc66a5cd Mon Sep 17 00:00:00 2001 From: Kevin Geiszler Date: Thu, 19 Mar 2026 18:01:02 -0700 Subject: [PATCH 1/2] HBASE-29965: Unable to dynamically change readonly flag Change-Id: I5b5479e37921ea233f586f0f02d2606320e16139 --- .../apache/hadoop/hbase/master/HMaster.java | 37 +++++++++----- .../hadoop/hbase/regionserver/HRegion.java | 48 ++++++++++++------- .../hbase/regionserver/HRegionServer.java | 33 ++++++++----- .../util/CoprocessorConfigurationUtil.java | 22 +++++---- ...tReadOnlyControllerCoprocessorLoading.java | 20 ++++---- .../TestCoprocessorConfigurationUtil.java | 38 +++++++-------- 6 files changed, 123 insertions(+), 75 deletions(-) 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 17355df9af1e..95562eded362 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 @@ -18,6 +18,8 @@ package org.apache.hadoop.hbase.master; import static org.apache.hadoop.hbase.HConstants.DEFAULT_HBASE_SPLIT_COORDINATED_BY_ZK; +import static org.apache.hadoop.hbase.HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT; +import static org.apache.hadoop.hbase.HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY; import static org.apache.hadoop.hbase.HConstants.HBASE_MASTER_LOGCLEANER_PLUGINS; import static org.apache.hadoop.hbase.HConstants.HBASE_SPLIT_WAL_COORDINATED_BY_ZK; import static org.apache.hadoop.hbase.master.cleaner.HFileCleaner.CUSTOM_POOL_SIZE; @@ -498,6 +500,8 @@ public class HMaster extends HBaseServerBase implements Maste public static final String WARMUP_BEFORE_MOVE = "hbase.master.warmup.before.move"; private static final boolean DEFAULT_WARMUP_BEFORE_MOVE = true; + private volatile boolean isGlobalReadOnlyEnabled; + /** * Use RSProcedureDispatcher instance to initiate master -> rs remote procedure execution. Use * this config to extend RSProcedureDispatcher (mainly for testing purpose). @@ -583,6 +587,8 @@ public HMaster(final Configuration conf) throws IOException { getChoreService().scheduleChore(clusterStatusPublisherChore); } } + this.isGlobalReadOnlyEnabled = + conf.getBoolean(HBASE_GLOBAL_READONLY_ENABLED_KEY, HBASE_GLOBAL_READONLY_ENABLED_DEFAULT); this.activeMasterManager = createActiveMasterManager(zooKeeper, serverName, this); cachedClusterId = new CachedClusterId(this, conf); this.regionServerTracker = new RegionServerTracker(zooKeeper, this); @@ -1090,8 +1096,7 @@ private void finishActiveMasterInitialization() throws IOException, InterruptedE if (!maintenanceMode) { startupTaskGroup.addTask("Initializing master coprocessors"); setQuotasObserver(conf); - CoprocessorConfigurationUtil.syncReadOnlyConfigurations( - ConfigurationUtil.isReadOnlyModeEnabled(conf), conf, + CoprocessorConfigurationUtil.syncReadOnlyConfigurations(conf, CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY); AbstractReadOnlyController.manageActiveClusterIdFile( ConfigurationUtil.isReadOnlyModeEnabled(conf), this.getMasterFileSystem()); @@ -4501,21 +4506,27 @@ public void onConfigurationChange(Configuration newConf) { // append the quotas observer back to the master coprocessor key setQuotasObserver(newConf); - boolean readOnlyMode = ConfigurationUtil.isReadOnlyModeEnabled(newConf); - CoprocessorConfigurationUtil.syncReadOnlyConfigurations(readOnlyMode, newConf, - CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY); + boolean maybeUpdatedReadOnlyMode = ConfigurationUtil.isReadOnlyModeEnabled(newConf); + boolean hasReadOnlyModeChanged = this.isGlobalReadOnlyEnabled != maybeUpdatedReadOnlyMode; + boolean hasCoprocessorConfigChanged = CoprocessorConfigurationUtil + .checkConfigurationChange(this.cpHost, newConf, CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY); + boolean shouldUpdateCoprocessors = + (hasCoprocessorConfigChanged || hasReadOnlyModeChanged) && !maintenanceMode; // update region server coprocessor if the configuration has changed. - if ( - CoprocessorConfigurationUtil.checkConfigurationChange(this.cpHost, newConf, - CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY) && !maintenanceMode - ) { + if (shouldUpdateCoprocessors) { LOG.info("Update the master coprocessor(s) because the configuration has changed"); - initializeCoprocessorHost(newConf); - CoprocessorConfigurationUtil.syncReadOnlyConfigurations(readOnlyMode, this.conf, + CoprocessorConfigurationUtil.syncReadOnlyConfigurations(newConf, CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY); - AbstractReadOnlyController.manageActiveClusterIdFile( - ConfigurationUtil.isReadOnlyModeEnabled(newConf), this.getMasterFileSystem()); + initializeCoprocessorHost(newConf); + } + + if (hasReadOnlyModeChanged) { + this.isGlobalReadOnlyEnabled = maybeUpdatedReadOnlyMode; + LOG.info("Config {} has been dynamically changed to {} for HMaster {}", + HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, this.isGlobalReadOnlyEnabled, this); + AbstractReadOnlyController.manageActiveClusterIdFile(this.isGlobalReadOnlyEnabled, + this.getMasterFileSystem()); } } 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 13eb59793595..2d6fc9bf7ae9 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 @@ -17,6 +17,8 @@ */ package org.apache.hadoop.hbase.regionserver; +import static org.apache.hadoop.hbase.HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT; +import static org.apache.hadoop.hbase.HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY; import static org.apache.hadoop.hbase.HConstants.REPLICATION_SCOPE_LOCAL; import static org.apache.hadoop.hbase.regionserver.HStoreFile.MAJOR_COMPACTION_KEY; import static org.apache.hadoop.hbase.trace.HBaseSemanticAttributes.REGION_NAMES_KEY; @@ -391,6 +393,8 @@ public class HRegion implements HeapSize, PropagatingConfigurationObserver, Regi private Path regionWalDir; private FileSystem walFS; + private volatile boolean isGlobalReadOnlyEnabled; + // set to true if the region is restored from snapshot for reading by ClientSideRegionScanner private boolean isRestoredRegion = false; @@ -941,8 +945,9 @@ public HRegion(final HRegionFileSystem fs, final WAL wal, final Configuration co decorateRegionConfiguration(conf); - CoprocessorConfigurationUtil.syncReadOnlyConfigurations( - ConfigurationUtil.isReadOnlyModeEnabled(conf), this.conf, + this.isGlobalReadOnlyEnabled = + conf.getBoolean(HBASE_GLOBAL_READONLY_ENABLED_KEY, HBASE_GLOBAL_READONLY_ENABLED_DEFAULT); + CoprocessorConfigurationUtil.syncReadOnlyConfigurations(this.conf, CoprocessorHost.REGION_COPROCESSOR_CONF_KEY); if (rsServices != null) { @@ -8515,7 +8520,7 @@ public boolean registerService(Service instance) { ServiceDescriptor serviceDesc = instance.getDescriptorForType(); String serviceName = CoprocessorRpcUtils.getServiceName(serviceDesc); if (coprocessorServiceHandlers.containsKey(serviceName)) { - LOG.error("Coprocessor service {} already registered, rejecting request from {} in region {}", + LOG.warn("Coprocessor service {} already registered, rejecting request from {} in region {}", serviceName, instance, this); return false; } @@ -8986,24 +8991,29 @@ IOException throwOnInterrupt(Throwable t) { * {@inheritDoc} */ @Override - public void onConfigurationChange(Configuration conf) { - this.storeHotnessProtector.update(conf); + public void onConfigurationChange(Configuration newConf) { + this.storeHotnessProtector.update(newConf); - boolean readOnlyMode = ConfigurationUtil.isReadOnlyModeEnabled(conf); - CoprocessorConfigurationUtil.syncReadOnlyConfigurations(readOnlyMode, conf, - CoprocessorHost.REGION_COPROCESSOR_CONF_KEY); + boolean maybeUpdatedReadOnlyMode = ConfigurationUtil.isReadOnlyModeEnabled(newConf); + boolean hasReadOnlyModeChanged = this.isGlobalReadOnlyEnabled != maybeUpdatedReadOnlyMode; + boolean hasCoprocessorConfigChanged = CoprocessorConfigurationUtil.checkConfigurationChange( + this.coprocessorHost, newConf, CoprocessorHost.REGION_COPROCESSOR_CONF_KEY, + CoprocessorHost.USER_REGION_COPROCESSOR_CONF_KEY); + boolean shouldUpdateCoprocessors = hasCoprocessorConfigChanged || hasReadOnlyModeChanged; // update coprocessorHost if the configuration has changed. - if ( - CoprocessorConfigurationUtil.checkConfigurationChange(this.coprocessorHost, conf, - CoprocessorHost.REGION_COPROCESSOR_CONF_KEY, - CoprocessorHost.USER_REGION_COPROCESSOR_CONF_KEY) - ) { + if (shouldUpdateCoprocessors) { LOG.info("Update the system coprocessors because the configuration has changed"); - decorateRegionConfiguration(conf); - this.coprocessorHost = new RegionCoprocessorHost(this, rsServices, conf); - CoprocessorConfigurationUtil.syncReadOnlyConfigurations(readOnlyMode, this.conf, + CoprocessorConfigurationUtil.syncReadOnlyConfigurations(newConf, CoprocessorHost.REGION_COPROCESSOR_CONF_KEY); + decorateRegionConfiguration(newConf); + this.coprocessorHost = new RegionCoprocessorHost(this, rsServices, newConf); + } + + if (hasReadOnlyModeChanged) { + this.isGlobalReadOnlyEnabled = maybeUpdatedReadOnlyMode; + LOG.info("Config {} has been dynamically changed to {} for region {}", + HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, this.isGlobalReadOnlyEnabled, this); } } @@ -9160,4 +9170,10 @@ public void addWriteRequestsCount(long writeRequestsCount) { boolean isReadsEnabled() { return this.writestate.readsEnabled; } + + @RestrictedApi(explanation = "Should only be called in tests", link = "", + allowedOnPath = ".*/src/test/.*") + public ConfigurationManager getConfigurationManager() { + return configurationManager; + } } 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 657cb01f38db..c593dcbf1749 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 @@ -20,6 +20,8 @@ import static org.apache.hadoop.hbase.HConstants.DEFAULT_HBASE_SPLIT_COORDINATED_BY_ZK; import static org.apache.hadoop.hbase.HConstants.DEFAULT_HBASE_SPLIT_WAL_MAX_SPLITTER; import static org.apache.hadoop.hbase.HConstants.DEFAULT_SLOW_LOG_SYS_TABLE_CHORE_DURATION; +import static org.apache.hadoop.hbase.HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT; +import static org.apache.hadoop.hbase.HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY; import static org.apache.hadoop.hbase.HConstants.HBASE_SPLIT_WAL_COORDINATED_BY_ZK; import static org.apache.hadoop.hbase.HConstants.HBASE_SPLIT_WAL_MAX_SPLITTER; import static org.apache.hadoop.hbase.master.waleventtracker.WALEventTrackerTableCreator.WAL_EVENT_TRACKER_ENABLED_DEFAULT; @@ -318,6 +320,7 @@ public class HRegionServer extends HBaseServerBase private LeaseManager leaseManager; private volatile boolean dataFsOk; + private volatile boolean isGlobalReadOnlyEnabled; static final String ABORT_TIMEOUT = "hbase.regionserver.abort.timeout"; // Default abort timeout is 1200 seconds for safe @@ -546,6 +549,9 @@ public HRegionServer(final Configuration conf) throws IOException { uncaughtExceptionHandler = (t, e) -> abort("Uncaught exception in executorService thread " + t.getName(), e); + this.isGlobalReadOnlyEnabled = + conf.getBoolean(HBASE_GLOBAL_READONLY_ENABLED_KEY, HBASE_GLOBAL_READONLY_ENABLED_DEFAULT); + // If no master in cluster, skip trying to track one or look for a cluster status. if (!this.masterless) { masterAddressTracker = new MasterAddressTracker(getZooKeeper(), this); @@ -827,9 +833,9 @@ public void run() { if (!isStopped() && !isAborted()) { installShutdownHook(); - CoprocessorConfigurationUtil.syncReadOnlyConfigurations( - ConfigurationUtil.isReadOnlyModeEnabled(conf), conf, + CoprocessorConfigurationUtil.syncReadOnlyConfigurations(conf, CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY); + // Initialize the RegionServerCoprocessorHost now that our ephemeral // node was created, in case any coprocessors want to use ZooKeeper this.rsHost = new RegionServerCoprocessorHost(this, this.conf); @@ -3488,19 +3494,24 @@ public void onConfigurationChange(Configuration newConf) { LOG.warn("Failed to initialize SuperUsers on reloading of the configuration"); } - boolean readOnlyMode = ConfigurationUtil.isReadOnlyModeEnabled(newConf); - CoprocessorConfigurationUtil.syncReadOnlyConfigurations(readOnlyMode, newConf, - CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY); + boolean maybeUpdatedReadOnlyMode = ConfigurationUtil.isReadOnlyModeEnabled(newConf); + boolean hasReadOnlyModeChanged = this.isGlobalReadOnlyEnabled != maybeUpdatedReadOnlyMode; + boolean hasCoprocessorConfigChanged = CoprocessorConfigurationUtil.checkConfigurationChange( + this.rsHost, newConf, CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY); + boolean shouldUpdateCoprocessors = hasCoprocessorConfigChanged || hasReadOnlyModeChanged; // update region server coprocessor if the configuration has changed. - if ( - CoprocessorConfigurationUtil.checkConfigurationChange(this.rsHost, newConf, - CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY) - ) { + if (shouldUpdateCoprocessors) { LOG.info("Update region server coprocessors because the configuration has changed"); - this.rsHost = new RegionServerCoprocessorHost(this, newConf); - CoprocessorConfigurationUtil.syncReadOnlyConfigurations(readOnlyMode, this.conf, + CoprocessorConfigurationUtil.syncReadOnlyConfigurations(newConf, CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY); + this.rsHost = new RegionServerCoprocessorHost(this, newConf); + } + + if (hasReadOnlyModeChanged) { + this.isGlobalReadOnlyEnabled = maybeUpdatedReadOnlyMode; + LOG.info("Config {} has been dynamically changed to {} for region server {}", + HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, this.isGlobalReadOnlyEnabled, this); } } 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 bbd1e6755d54..6e1fd049c271 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 @@ -175,16 +175,22 @@ private static List getReadOnlyCoprocessors(String configurationKey) { }; } - public static void syncReadOnlyConfigurations(boolean readOnlyMode, Configuration conf, - String configurationKey) { - conf.setBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, readOnlyMode); + /** + * This method adds or removes relevant ReadOnlyController coprocessors to the provided + * configuration based on whether read-only mode is enabled. + * @param conf The up-to-date configuration used to determine how to handle + * coprocessors + * @param coprocessorConfKey The configuration key name + */ + public static void syncReadOnlyConfigurations(Configuration conf, String coprocessorConfKey) { + boolean isReadOnlyModeEnabled = conf.getBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, + HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT); - List cpList = getReadOnlyCoprocessors(configurationKey); - // If readonly is true then add the coprocessor of master - if (readOnlyMode) { - CoprocessorConfigurationUtil.addCoprocessors(conf, configurationKey, cpList); + List cpList = getReadOnlyCoprocessors(coprocessorConfKey); + if (isReadOnlyModeEnabled) { + CoprocessorConfigurationUtil.addCoprocessors(conf, coprocessorConfKey, cpList); } else { - CoprocessorConfigurationUtil.removeCoprocessors(conf, configurationKey, cpList); + CoprocessorConfigurationUtil.removeCoprocessors(conf, coprocessorConfKey, cpList); } } } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestReadOnlyControllerCoprocessorLoading.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestReadOnlyControllerCoprocessorLoading.java index 4846e84bcd62..c9692fb751e5 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestReadOnlyControllerCoprocessorLoading.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestReadOnlyControllerCoprocessorLoading.java @@ -107,8 +107,8 @@ private void createTable() throws Exception { region = regions.get(0); } - private void setReadOnlyMode(boolean isReadOnlyEnabled) { - // Create a new configuration to micic client server behavior + private Configuration setReadOnlyMode(boolean isReadOnlyEnabled) { + // Create a new configuration to mimic client server behavior // otherwise the existing conf object is shared with the cluster // and can cause side effects on other tests if not reset properly. // This way we can ensure that only the coprocessor loading is tested @@ -119,9 +119,10 @@ private void setReadOnlyMode(boolean isReadOnlyEnabled) { newConf.setBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, isReadOnlyEnabled); master.getConfigurationManager().notifyAllObservers(newConf); regionServer.getConfigurationManager().notifyAllObservers(newConf); + return newConf; } - private void verifyMasterReadOnlyControllerLoading(boolean isReadOnlyEnabled) throws Exception { + private void verifyMasterReadOnlyControllerLoading(boolean isReadOnlyEnabled) { MasterCoprocessorHost masterCPHost = master.getMasterCoprocessorHost(); if (isReadOnlyEnabled) { assertNotNull( @@ -136,8 +137,7 @@ private void verifyMasterReadOnlyControllerLoading(boolean isReadOnlyEnabled) th } } - private void verifyRegionServerReadOnlyControllerLoading(boolean isReadOnlyEnabled) - throws Exception { + private void verifyRegionServerReadOnlyControllerLoading(boolean isReadOnlyEnabled) { RegionServerCoprocessorHost rsCPHost = regionServer.getRegionServerCoprocessorHost(); if (isReadOnlyEnabled) { assertNotNull( @@ -152,7 +152,7 @@ private void verifyRegionServerReadOnlyControllerLoading(boolean isReadOnlyEnabl } } - private void verifyRegionReadOnlyControllerLoading(boolean isReadOnlyEnabled) throws Exception { + private void verifyRegionReadOnlyControllerLoading(boolean isReadOnlyEnabled) { RegionCoprocessorHost regionCPHost = region.getCoprocessorHost(); if (isReadOnlyEnabled) { @@ -220,8 +220,10 @@ public void testReadOnlyControllerLoadedWhenEnabledDynamically() throws Exceptio public void testReadOnlyControllerUnloadedWhenDisabledDynamically() throws Exception { setupMiniCluster(initialReadOnlyMode); boolean isReadOnlyEnabled = false; - setReadOnlyMode(isReadOnlyEnabled); + Configuration newConf = setReadOnlyMode(isReadOnlyEnabled); createTable(); + // The newly created table's region has a stale conf that needs to be updated + region.onConfigurationChange(newConf); verifyMasterReadOnlyControllerLoading(isReadOnlyEnabled); verifyRegionServerReadOnlyControllerLoading(isReadOnlyEnabled); verifyRegionReadOnlyControllerLoading(isReadOnlyEnabled); @@ -232,8 +234,10 @@ public void testReadOnlyControllerLoadUnloadedWhenMultipleReadOnlyToggle() throw setupMiniCluster(initialReadOnlyMode); // Ensure region exists before validation - setReadOnlyMode(false); + Configuration newConf = setReadOnlyMode(false); createTable(); + // The newly created table's region has a stale conf that needs to be updated + region.onConfigurationChange(newConf); verifyReadOnlyState(false); // Define toggle sequence diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestCoprocessorConfigurationUtil.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestCoprocessorConfigurationUtil.java index eeda71949824..ad4363123303 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestCoprocessorConfigurationUtil.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestCoprocessorConfigurationUtil.java @@ -17,9 +17,9 @@ */ package org.apache.hadoop.hbase.util; +import static org.apache.hadoop.hbase.HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY; import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; @@ -27,7 +27,6 @@ import java.util.Arrays; import java.util.List; import org.apache.hadoop.conf.Configuration; -import org.apache.hadoop.hbase.HConstants; import org.apache.hadoop.hbase.coprocessor.CoprocessorHost; import org.apache.hadoop.hbase.security.access.BulkLoadReadOnlyController; import org.apache.hadoop.hbase.security.access.EndpointReadOnlyController; @@ -121,9 +120,9 @@ public void testRemoveCoprocessorsIdempotentWhenNotPresent() { assertArrayEquals(new String[] { "cp1" }, conf.getStrings(key)); } - private void assertEnable(String key, List expected) { - CoprocessorConfigurationUtil.syncReadOnlyConfigurations(true, conf, key); - assertTrue(conf.getBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, false)); + private void assertEnableReadOnlyMode(String key, List expected) { + conf.setBoolean(HBASE_GLOBAL_READONLY_ENABLED_KEY, true); + CoprocessorConfigurationUtil.syncReadOnlyConfigurations(conf, key); String[] result = conf.getStrings(key); assertNotNull(result); assertEquals(expected.size(), result.length); @@ -132,34 +131,33 @@ private void assertEnable(String key, List expected) { @Test public void testSyncReadOnlyConfigurationsReadOnlyEnableAllKeys() { - assertEnable(CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY, + assertEnableReadOnlyMode(CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY, List.of(MasterReadOnlyController.class.getName())); - assertEnable(CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY, + assertEnableReadOnlyMode(CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY, List.of(RegionServerReadOnlyController.class.getName())); - assertEnable(CoprocessorHost.REGION_COPROCESSOR_CONF_KEY, + assertEnableReadOnlyMode(CoprocessorHost.REGION_COPROCESSOR_CONF_KEY, List.of(RegionReadOnlyController.class.getName(), BulkLoadReadOnlyController.class.getName(), EndpointReadOnlyController.class.getName())); } - private void assertDisable(String key, List initialCoprocs) { + private void assertDisableReadOnlyMode(String key, List initialCoprocs) { conf.setStrings(key, initialCoprocs.toArray(new String[0])); - conf.setBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, true); - CoprocessorConfigurationUtil.syncReadOnlyConfigurations(false, conf, key); - assertFalse(conf.getBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, true)); + conf.setBoolean(HBASE_GLOBAL_READONLY_ENABLED_KEY, false); + CoprocessorConfigurationUtil.syncReadOnlyConfigurations(conf, key); String[] result = conf.getStrings(key); assertTrue(result == null || result.length == 0); } @Test public void testSyncReadOnlyConfigurationsReadOnlyDisableAllKeys() { - assertDisable(CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY, + assertDisableReadOnlyMode(CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY, List.of(MasterReadOnlyController.class.getName())); - assertDisable(CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY, + assertDisableReadOnlyMode(CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY, List.of(RegionServerReadOnlyController.class.getName())); - assertDisable(CoprocessorHost.REGION_COPROCESSOR_CONF_KEY, + assertDisableReadOnlyMode(CoprocessorHost.REGION_COPROCESSOR_CONF_KEY, List.of(RegionReadOnlyController.class.getName(), BulkLoadReadOnlyController.class.getName(), EndpointReadOnlyController.class.getName())); } @@ -168,7 +166,8 @@ public void testSyncReadOnlyConfigurationsReadOnlyDisableAllKeys() { public void testSyncReadOnlyConfigurationsReadOnlyEnablePreservesExistingCoprocessors() { String key = CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY; conf.setStrings(key, "existingCp"); - CoprocessorConfigurationUtil.syncReadOnlyConfigurations(true, conf, key); + conf.setBoolean(HBASE_GLOBAL_READONLY_ENABLED_KEY, true); + CoprocessorConfigurationUtil.syncReadOnlyConfigurations(conf, key); List result = Arrays.asList(conf.getStrings(key)); assertTrue(result.contains("existingCp")); assertTrue(result.contains(MasterReadOnlyController.class.getName())); @@ -180,7 +179,7 @@ public void testSyncReadOnlyConfigurationsReadOnlyDisableRemovesOnlyReadOnlyCopr String key = CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY; String existingCp = "org.example.OtherCP"; conf.setStrings(key, existingCp, MasterReadOnlyController.class.getName()); - CoprocessorConfigurationUtil.syncReadOnlyConfigurations(false, conf, key); + CoprocessorConfigurationUtil.syncReadOnlyConfigurations(conf, key); String[] cps = conf.getStrings(key); assertNotNull(cps); assertEquals(1, cps.length); @@ -190,9 +189,10 @@ public void testSyncReadOnlyConfigurationsReadOnlyDisableRemovesOnlyReadOnlyCopr @Test public void testSyncReadOnlyConfigurationsIsIdempotent() { Configuration conf = new Configuration(false); + conf.setBoolean(HBASE_GLOBAL_READONLY_ENABLED_KEY, true); String key = CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY; - CoprocessorConfigurationUtil.syncReadOnlyConfigurations(true, conf, key); - CoprocessorConfigurationUtil.syncReadOnlyConfigurations(true, conf, key); + CoprocessorConfigurationUtil.syncReadOnlyConfigurations(conf, key); + CoprocessorConfigurationUtil.syncReadOnlyConfigurations(conf, key); String[] cps = conf.getStrings(key); assertNotNull(cps); assertEquals(1, cps.length); From 084775c15279b0b67ca1af1a0372f38aa9eaae5f Mon Sep 17 00:00:00 2001 From: Kevin Geiszler Date: Fri, 20 Mar 2026 17:59:51 -0700 Subject: [PATCH 2/2] Add coprocessor logging to onConfigurationChange() Change-Id: Ie9ba9c771b12793516a6227888caf9c734e05c86 --- .../main/java/org/apache/hadoop/hbase/master/HMaster.java | 8 ++++++++ .../org/apache/hadoop/hbase/regionserver/HRegion.java | 8 ++++++++ .../apache/hadoop/hbase/regionserver/HRegionServer.java | 8 ++++++++ 3 files changed, 24 insertions(+) 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 95562eded362..5c52e2dbdeee 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 @@ -4515,10 +4515,18 @@ public void onConfigurationChange(Configuration newConf) { // update region server coprocessor if the configuration has changed. if (shouldUpdateCoprocessors) { + Set currentlyLoadedCps = this.cpHost.getCoprocessorClassNames(); + LOG.debug("About to update coprocessors loaded on HMaster {}. These are the current " + + "coprocessors before updating: {}", this, currentlyLoadedCps); + LOG.info("Update the master coprocessor(s) because the configuration has changed"); CoprocessorConfigurationUtil.syncReadOnlyConfigurations(newConf, CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY); initializeCoprocessorHost(newConf); + + currentlyLoadedCps = this.cpHost.getCoprocessorClassNames(); + LOG.debug("Finished updating coprocessors on HMaster {}. These are the coprocessors " + + "after updating: {}", this, currentlyLoadedCps); } if (hasReadOnlyModeChanged) { 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 2d6fc9bf7ae9..58cb529f9872 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 @@ -9003,11 +9003,19 @@ public void onConfigurationChange(Configuration newConf) { // update coprocessorHost if the configuration has changed. if (shouldUpdateCoprocessors) { + Set currentlyLoadedCps = this.coprocessorHost.getCoprocessorClassNames(); + LOG.trace("About to update coprocessors loaded on HRegion {}. These are the current " + + "coprocessors before updating: {}", this, currentlyLoadedCps); + LOG.info("Update the system coprocessors because the configuration has changed"); CoprocessorConfigurationUtil.syncReadOnlyConfigurations(newConf, CoprocessorHost.REGION_COPROCESSOR_CONF_KEY); decorateRegionConfiguration(newConf); this.coprocessorHost = new RegionCoprocessorHost(this, rsServices, newConf); + + currentlyLoadedCps = this.coprocessorHost.getCoprocessorClassNames(); + LOG.trace("Finished updating coprocessors on HRegion {}. These are the coprocessors " + + "after updating: {}", this, currentlyLoadedCps); } if (hasReadOnlyModeChanged) { 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 c593dcbf1749..51a23b9c0f07 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 @@ -3502,10 +3502,18 @@ public void onConfigurationChange(Configuration newConf) { // update region server coprocessor if the configuration has changed. if (shouldUpdateCoprocessors) { + Set currentlyLoadedCps = this.rsHost.getCoprocessorClassNames(); + LOG.debug("About to update coprocessors loaded on HRegionServer {}. These are the current " + + "coprocessors before updating: {}", this, currentlyLoadedCps); + LOG.info("Update region server coprocessors because the configuration has changed"); CoprocessorConfigurationUtil.syncReadOnlyConfigurations(newConf, CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY); this.rsHost = new RegionServerCoprocessorHost(this, newConf); + + currentlyLoadedCps = this.rsHost.getCoprocessorClassNames(); + LOG.debug("Finished updating coprocessors on HRegionServer {}. These are the coprocessors " + + "after updating: {}", this, currentlyLoadedCps); } if (hasReadOnlyModeChanged) {