This is an automated email from the ASF dual-hosted git repository. andor pushed a commit to branch HBASE-29081 in repository https://gitbox.apache.org/repos/asf/hbase.git
commit 6ad2a57a850afc01bc22b09572c6b0216744456b Author: Kevin Geiszler <[email protected]> AuthorDate: Mon Mar 30 14:12:05 2026 -0700 HBASE-29965: Unable to dynamically change readonly flag (#7964) * HBASE-29965: Unable to dynamically change readonly flag Change-Id: I5b5479e37921ea233f586f0f02d2606320e16139 * Refactor repeated code Change-Id: I9a0269b786f7282686d60ceff47a538d2b0b88fa * Add docstrings Change-Id: I3b456e0b2689dfad09d1f5a4b47fe8fd85d06bf9 --- .../hbase/coprocessor/CoprocessorReloadTask.java | 42 +++++++++++++ .../org/apache/hadoop/hbase/master/HMaster.java | 32 +++++----- .../apache/hadoop/hbase/regionserver/HRegion.java | 46 +++++++------- .../hadoop/hbase/regionserver/HRegionServer.java | 29 ++++----- .../hbase/util/CoprocessorConfigurationUtil.java | 70 +++++++++++++++++++--- .../TestReadOnlyControllerCoprocessorLoading.java | 20 ++++--- .../util/TestCoprocessorConfigurationUtil.java | 38 ++++++------ 7 files changed, 186 insertions(+), 91 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorReloadTask.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorReloadTask.java new file mode 100644 index 00000000000..0b826de333d --- /dev/null +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorReloadTask.java @@ -0,0 +1,42 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hbase.coprocessor; + +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hbase.HBaseInterfaceAudience; +import org.apache.yetus.audience.InterfaceAudience; +import org.apache.yetus.audience.InterfaceStability; + +/** + * This interface is used to perform whatever task is necessary for reloading coprocessors on + * HMaster, HRegionServer, and HRegion. Since the steps required to reload coprocessors varies for + * each of these types, this interface helps with code flexibility by allowing a lamda function to + * be provided for the {@link #reload(Configuration) reload()} method. <br> + * <br> + * See {@link org.apache.hadoop.hbase.util.CoprocessorConfigurationUtil#maybeUpdateCoprocessors + * CoprocessorConfigurationUtil.maybeUpdateCoprocessors()} and its usage in + * {@link org.apache.hadoop.hbase.conf.ConfigurationObserver#onConfigurationChange + * onConfigurationChange()} with HMaster, HRegionServer, and HRegion for an idea of how this + * interface is helpful. + */ [email protected](HBaseInterfaceAudience.COPROC) [email protected] +@FunctionalInterface +public interface CoprocessorReloadTask { + void reload(Configuration conf); +} 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 17355df9af1..8fd133f64f6 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<MasterRpcServices> 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 class HMaster extends HBaseServerBase<MasterRpcServices> implements Maste 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 @@ public class HMaster extends HBaseServerBase<MasterRpcServices> implements Maste 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,16 @@ public class HMaster extends HBaseServerBase<MasterRpcServices> implements Maste // 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 originalIsReadOnlyEnabled = this.isGlobalReadOnlyEnabled; - // update region server coprocessor if the configuration has changed. - if ( - 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); - CoprocessorConfigurationUtil.syncReadOnlyConfigurations(readOnlyMode, this.conf, - CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY); - AbstractReadOnlyController.manageActiveClusterIdFile( - ConfigurationUtil.isReadOnlyModeEnabled(newConf), this.getMasterFileSystem()); + CoprocessorConfigurationUtil.maybeUpdateCoprocessors(newConf, this.isGlobalReadOnlyEnabled, + this.cpHost, CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY, this.maintenanceMode, + this.toString(), val -> this.isGlobalReadOnlyEnabled = val, + conf -> initializeCoprocessorHost(newConf)); + + if (this.isGlobalReadOnlyEnabled != originalIsReadOnlyEnabled) { + 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 13eb5979359..5d1246645a4 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; @@ -178,7 +180,6 @@ import org.apache.hadoop.hbase.util.Bytes; import org.apache.hadoop.hbase.util.CancelableProgressable; import org.apache.hadoop.hbase.util.ClassSize; import org.apache.hadoop.hbase.util.CommonFSUtils; -import org.apache.hadoop.hbase.util.ConfigurationUtil; import org.apache.hadoop.hbase.util.CoprocessorConfigurationUtil; import org.apache.hadoop.hbase.util.EnvironmentEdgeManager; import org.apache.hadoop.hbase.util.FSUtils; @@ -391,6 +392,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 +944,9 @@ public class HRegion implements HeapSize, PropagatingConfigurationObserver, Regi 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 +8519,7 @@ public class HRegion implements HeapSize, PropagatingConfigurationObserver, Regi 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,25 +8990,15 @@ public class HRegion implements HeapSize, PropagatingConfigurationObserver, Regi * {@inheritDoc} */ @Override - public void onConfigurationChange(Configuration conf) { - this.storeHotnessProtector.update(conf); - - boolean readOnlyMode = ConfigurationUtil.isReadOnlyModeEnabled(conf); - CoprocessorConfigurationUtil.syncReadOnlyConfigurations(readOnlyMode, conf, - CoprocessorHost.REGION_COPROCESSOR_CONF_KEY); - - // update coprocessorHost if the configuration has changed. - if ( - CoprocessorConfigurationUtil.checkConfigurationChange(this.coprocessorHost, conf, - CoprocessorHost.REGION_COPROCESSOR_CONF_KEY, - CoprocessorHost.USER_REGION_COPROCESSOR_CONF_KEY) - ) { - 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, - CoprocessorHost.REGION_COPROCESSOR_CONF_KEY); - } + public void onConfigurationChange(Configuration newConf) { + this.storeHotnessProtector.update(newConf); + + CoprocessorConfigurationUtil.maybeUpdateCoprocessors(newConf, this.isGlobalReadOnlyEnabled, + this.coprocessorHost, CoprocessorHost.REGION_COPROCESSOR_CONF_KEY, false, this.toString(), + val -> this.isGlobalReadOnlyEnabled = val, conf -> { + decorateRegionConfiguration(conf); + this.coprocessorHost = new RegionCoprocessorHost(this, rsServices, newConf); + }); } /** @@ -9160,4 +9154,10 @@ public class HRegion implements HeapSize, PropagatingConfigurationObserver, Regi 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 657cb01f38d..8cd6f738b33 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 @@ package org.apache.hadoop.hbase.regionserver; 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; @@ -161,7 +163,6 @@ import org.apache.hadoop.hbase.security.UserProvider; import org.apache.hadoop.hbase.trace.TraceUtil; import org.apache.hadoop.hbase.util.Bytes; import org.apache.hadoop.hbase.util.CompressionTest; -import org.apache.hadoop.hbase.util.ConfigurationUtil; import org.apache.hadoop.hbase.util.CoprocessorConfigurationUtil; import org.apache.hadoop.hbase.util.EnvironmentEdgeManager; import org.apache.hadoop.hbase.util.FSUtils; @@ -318,6 +319,7 @@ public class HRegionServer extends HBaseServerBase<RSRpcServices> 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 +548,9 @@ public class HRegionServer extends HBaseServerBase<RSRpcServices> 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 +832,9 @@ public class HRegionServer extends HBaseServerBase<RSRpcServices> 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,20 +3493,10 @@ public class HRegionServer extends HBaseServerBase<RSRpcServices> 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); - - // update region server coprocessor if the configuration has changed. - if ( - CoprocessorConfigurationUtil.checkConfigurationChange(this.rsHost, newConf, - CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY) - ) { - LOG.info("Update region server coprocessors because the configuration has changed"); - this.rsHost = new RegionServerCoprocessorHost(this, newConf); - CoprocessorConfigurationUtil.syncReadOnlyConfigurations(readOnlyMode, this.conf, - CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY); - } + CoprocessorConfigurationUtil.maybeUpdateCoprocessors(newConf, this.isGlobalReadOnlyEnabled, + this.rsHost, CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY, false, this.toString(), + val -> this.isGlobalReadOnlyEnabled = val, + conf -> this.rsHost = new RegionServerCoprocessorHost(this, newConf)); } @Override 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 bbd1e6755d5..46eb6957105 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 @@ -22,15 +22,19 @@ import java.util.Arrays; import java.util.HashSet; import java.util.List; import java.util.Set; +import java.util.function.Consumer; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hbase.HConstants; import org.apache.hadoop.hbase.coprocessor.CoprocessorHost; +import org.apache.hadoop.hbase.coprocessor.CoprocessorReloadTask; import org.apache.hadoop.hbase.security.access.BulkLoadReadOnlyController; import org.apache.hadoop.hbase.security.access.EndpointReadOnlyController; import org.apache.hadoop.hbase.security.access.MasterReadOnlyController; import org.apache.hadoop.hbase.security.access.RegionReadOnlyController; import org.apache.hadoop.hbase.security.access.RegionServerReadOnlyController; import org.apache.yetus.audience.InterfaceAudience; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.apache.hbase.thirdparty.com.google.common.base.Preconditions; import org.apache.hbase.thirdparty.com.google.common.base.Strings; @@ -40,6 +44,7 @@ import org.apache.hbase.thirdparty.com.google.common.base.Strings; */ @InterfaceAudience.Private public final class CoprocessorConfigurationUtil { + private static final Logger LOG = LoggerFactory.getLogger(CoprocessorConfigurationUtil.class); private CoprocessorConfigurationUtil() { } @@ -175,16 +180,65 @@ public final class CoprocessorConfigurationUtil { }; } - 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<String> cpList = getReadOnlyCoprocessors(configurationKey); - // If readonly is true then add the coprocessor of master - if (readOnlyMode) { - CoprocessorConfigurationUtil.addCoprocessors(conf, configurationKey, cpList); + List<String> cpList = getReadOnlyCoprocessors(coprocessorConfKey); + if (isReadOnlyModeEnabled) { + CoprocessorConfigurationUtil.addCoprocessors(conf, coprocessorConfKey, cpList); } else { - CoprocessorConfigurationUtil.removeCoprocessors(conf, configurationKey, cpList); + CoprocessorConfigurationUtil.removeCoprocessors(conf, coprocessorConfKey, cpList); + } + } + + /** + * This method updates the coprocessors on the master, region server, or region if a change has + * been detected. Detected changes include changes in coprocessors or changes in read-only mode + * configuration. If a change is detected, then new coprocessors are loaded using the provided + * reload method. The new value for the read-only config variable is updated as well. + * @param newConf an updated configuration + * @param originalIsReadOnlyEnabled the original value for + * {@value HConstants#HBASE_GLOBAL_READONLY_ENABLED_KEY} + * @param coprocessorHost the coprocessor host for HMaster, HRegionServer, or HRegion + * @param coprocessorConfKey configuration key used for setting master, region server, or + * region coprocessors + * @param isMaintenanceMode whether maintenance mode is active (mainly for HMaster) + * @param instance string value of the instance calling this method (mainly helps + * with tracking region logging) + * @param stateSetter lambda function that sets the read-only instance variable with + * an updated value from the config + * @param reloadTask lambda function that reloads coprocessors on the master, + * region server, or region + */ + public static void maybeUpdateCoprocessors(Configuration newConf, + boolean originalIsReadOnlyEnabled, CoprocessorHost<?, ?> coprocessorHost, + String coprocessorConfKey, boolean isMaintenanceMode, String instance, + Consumer<Boolean> stateSetter, CoprocessorReloadTask reloadTask) { + + boolean maybeUpdatedReadOnlyMode = ConfigurationUtil.isReadOnlyModeEnabled(newConf); + boolean hasReadOnlyModeChanged = originalIsReadOnlyEnabled != maybeUpdatedReadOnlyMode; + boolean hasCoprocessorConfigChanged = CoprocessorConfigurationUtil + .checkConfigurationChange(coprocessorHost, newConf, coprocessorConfKey); + + // update region server coprocessor if the configuration has changed. + if ((hasCoprocessorConfigChanged || hasReadOnlyModeChanged) && !isMaintenanceMode) { + LOG.info("Updating coprocessors for {} because the configuration has changed", instance); + CoprocessorConfigurationUtil.syncReadOnlyConfigurations(newConf, coprocessorConfKey); + reloadTask.reload(newConf); + } + + if (hasReadOnlyModeChanged) { + stateSetter.accept(maybeUpdatedReadOnlyMode); + LOG.info("Config {} has been dynamically changed to {} for {}", + HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, maybeUpdatedReadOnlyMode, instance); } } } 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 4846e84bcd6..c9692fb751e 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 @@ public class TestReadOnlyControllerCoprocessorLoading { 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 @@ public class TestReadOnlyControllerCoprocessorLoading { 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 @@ public class TestReadOnlyControllerCoprocessorLoading { } } - private void verifyRegionServerReadOnlyControllerLoading(boolean isReadOnlyEnabled) - throws Exception { + private void verifyRegionServerReadOnlyControllerLoading(boolean isReadOnlyEnabled) { RegionServerCoprocessorHost rsCPHost = regionServer.getRegionServerCoprocessorHost(); if (isReadOnlyEnabled) { assertNotNull( @@ -152,7 +152,7 @@ public class TestReadOnlyControllerCoprocessorLoading { } } - private void verifyRegionReadOnlyControllerLoading(boolean isReadOnlyEnabled) throws Exception { + private void verifyRegionReadOnlyControllerLoading(boolean isReadOnlyEnabled) { RegionCoprocessorHost regionCPHost = region.getCoprocessorHost(); if (isReadOnlyEnabled) { @@ -220,8 +220,10 @@ public class TestReadOnlyControllerCoprocessorLoading { 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 class TestReadOnlyControllerCoprocessorLoading { 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 16b68388ed5..273be065e01 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 static org.junit.Assert.assertTrue; 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; @@ -125,9 +124,9 @@ public class TestCoprocessorConfigurationUtil { assertArrayEquals(new String[] { "cp1" }, conf.getStrings(key)); } - private void assertEnable(String key, List<String> expected) { - CoprocessorConfigurationUtil.syncReadOnlyConfigurations(true, conf, key); - assertTrue(conf.getBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, false)); + private void assertEnableReadOnlyMode(String key, List<String> 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); @@ -136,34 +135,33 @@ public class TestCoprocessorConfigurationUtil { @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<String> initialCoprocs) { + private void assertDisableReadOnlyMode(String key, List<String> 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())); } @@ -172,7 +170,8 @@ public class TestCoprocessorConfigurationUtil { 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<String> result = Arrays.asList(conf.getStrings(key)); assertTrue(result.contains("existingCp")); assertTrue(result.contains(MasterReadOnlyController.class.getName())); @@ -184,7 +183,7 @@ public class TestCoprocessorConfigurationUtil { 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); @@ -194,9 +193,10 @@ public class TestCoprocessorConfigurationUtil { @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);
