From 4899a4ef3560d020af462325e54767e549e0ee8d Mon Sep 17 00:00:00 2001 From: Kevin Geiszler Date: Tue, 14 Apr 2026 11:42:51 -0700 Subject: [PATCH 1/4] Address coprocessor and dynamic configuration review comments Change-Id: Ib7ff9d449f7368153bc67f0bd2d6a7ca591900e8 --- .../org/apache/hadoop/hbase/HConstants.java | 4 +- .../hbase/coprocessor/CoprocessorHost.java | 35 --------- .../apache/hadoop/hbase/master/HMaster.java | 35 ++++----- .../hadoop/hbase/master/MasterFileSystem.java | 2 +- .../hbase/regionserver/CompactSplit.java | 4 +- .../hadoop/hbase/regionserver/HRegion.java | 29 ++----- .../hbase/regionserver/HRegionServer.java | 23 +++--- .../hadoop/hbase/util/ConfigurationUtil.java | 2 +- .../util/CoprocessorConfigurationUtil.java | 77 +++++++++++++++---- .../access/TestReadOnlyController.java | 4 +- ...tReadOnlyControllerCoprocessorLoading.java | 3 + 11 files changed, 106 insertions(+), 112 deletions(-) diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java index 4ed6d3dc9d58..21e892b69737 100644 --- a/hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java +++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java @@ -1666,9 +1666,7 @@ public enum OperationStatusCode { public final static boolean REJECT_DECOMMISSIONED_HOSTS_DEFAULT = false; /** - * Adds a suffix to the meta table name: value=’test’ -> ‘hbase:meta_test’ Added in HBASE-XXXXX to - * support having multiple hbase:meta tables (with distinct names )to enable storage sharing by - * more than one clusters. + * Adds a suffix to the meta table name: value='test' -> ‘hbase:meta_test’ */ public final static String HBASE_META_TABLE_SUFFIX = "hbase.meta.table.suffix"; 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 625ac1dc5842..3d5897c0a056 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 @@ -36,8 +36,6 @@ import org.apache.hadoop.hbase.CoprocessorEnvironment; import org.apache.hadoop.hbase.DoNotRetryIOException; import org.apache.hadoop.hbase.HConstants; -import org.apache.hadoop.hbase.conf.ConfigurationManager; -import org.apache.hadoop.hbase.conf.ConfigurationObserver; import org.apache.hadoop.hbase.ipc.RpcServer; import org.apache.hadoop.hbase.security.User; import org.apache.hadoop.hbase.util.CoprocessorClassLoader; @@ -132,39 +130,6 @@ public Set getCoprocessorClassNames() { return returnValue; } - /** - * Used to help make the relevant loaded coprocessors dynamically configurable by registering them - * to the {@link ConfigurationManager}. Coprocessors are considered "relevant" if they implement - * the {@link ConfigurationObserver} interface. - * @param configurationManager the ConfigurationManager the coprocessors get registered to - */ - public void registerConfigurationObservers(ConfigurationManager configurationManager) { - Coprocessor foundCp; - Set coprocessors = this.getCoprocessors(); - for (String cp : coprocessors) { - foundCp = this.findCoprocessor(cp); - if (foundCp instanceof ConfigurationObserver) { - configurationManager.registerObserver((ConfigurationObserver) foundCp); - } - } - } - - /** - * Deregisters relevant coprocessors from the {@link ConfigurationManager}. Coprocessors are - * considered "relevant" if they implement the {@link ConfigurationObserver} interface. - * @param configurationManager the ConfigurationManager the coprocessors get deregistered from - */ - public void deregisterConfigurationObservers(ConfigurationManager configurationManager) { - Coprocessor foundCp; - Set coprocessors = this.getCoprocessors(); - for (String cp : coprocessors) { - foundCp = this.findCoprocessor(cp); - if (foundCp instanceof ConfigurationObserver) { - configurationManager.deregisterObserver((ConfigurationObserver) foundCp); - } - } - } - /** * 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 8fd133f64f68..ba00ed38defe 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,8 +18,6 @@ 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; @@ -500,8 +498,6 @@ 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). @@ -587,8 +583,6 @@ 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); @@ -622,12 +616,6 @@ protected String getUseThisHostnameInstead(Configuration conf) { private void registerConfigurationObservers() { configurationManager.registerObserver(this.rpcServices); configurationManager.registerObserver(this); - if (cpHost != null) { - cpHost.registerConfigurationObservers(configurationManager); - } else { - LOG.warn("Could not register HMaster coprocessors to the ConfigurationManager because " - + "MasterCoprocessorHost is null"); - } } // Main run loop. Calls through to the regionserver run loop AFTER becoming active Master; will @@ -636,6 +624,7 @@ private void registerConfigurationObservers() { public void run() { try { installShutdownHook(); + registerConfigurationObservers(); Threads.setDaemonThreadRunning(new Thread(TraceUtil.tracedRunnable(() -> { try { int infoPort = putUpJettyServer(); @@ -1099,7 +1088,7 @@ private void finishActiveMasterInitialization() throws IOException, InterruptedE CoprocessorConfigurationUtil.syncReadOnlyConfigurations(conf, CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY); AbstractReadOnlyController.manageActiveClusterIdFile( - ConfigurationUtil.isReadOnlyModeEnabled(conf), this.getMasterFileSystem()); + ConfigurationUtil.isReadOnlyModeEnabledInConf(conf), this.getMasterFileSystem()); initializeCoprocessorHost(conf); } else { // start an in process region server for carrying system regions @@ -4506,15 +4495,22 @@ public void onConfigurationChange(Configuration newConf) { // append the quotas observer back to the master coprocessor key setQuotasObserver(newConf); - boolean originalIsReadOnlyEnabled = this.isGlobalReadOnlyEnabled; + boolean originalIsReadOnlyEnabled = CoprocessorConfigurationUtil + .areReadOnlyCoprocessorsLoaded(this.conf, CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY); - CoprocessorConfigurationUtil.maybeUpdateCoprocessors(newConf, this.isGlobalReadOnlyEnabled, + CoprocessorConfigurationUtil.maybeUpdateCoprocessors(newConf, originalIsReadOnlyEnabled, this.cpHost, CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY, this.maintenanceMode, - this.toString(), val -> this.isGlobalReadOnlyEnabled = val, - conf -> initializeCoprocessorHost(newConf)); + this.toString(), conf -> { + this.initializeCoprocessorHost(conf); + CoprocessorConfigurationUtil.updateCoprocessorListInConf(this.conf, conf, + CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY); + }); + + boolean maybeUpdatedReadOnlyMode = CoprocessorConfigurationUtil + .areReadOnlyCoprocessorsLoaded(this.conf, CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY); - if (this.isGlobalReadOnlyEnabled != originalIsReadOnlyEnabled) { - AbstractReadOnlyController.manageActiveClusterIdFile(this.isGlobalReadOnlyEnabled, + if (maybeUpdatedReadOnlyMode != originalIsReadOnlyEnabled) { + AbstractReadOnlyController.manageActiveClusterIdFile(maybeUpdatedReadOnlyMode, this.getMasterFileSystem()); } } @@ -4617,7 +4613,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); - registerConfigurationObservers(); } @Override diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java index a6b61d4c005b..d0b622a8bec1 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java @@ -396,7 +396,7 @@ public void logFileSystemState(Logger log) throws IOException { private void negotiateActiveClusterSuffixFile(long wait) throws IOException { this.activeClusterSuffix = ActiveClusterSuffix.fromConfig(conf, getClusterId()); - if (!ConfigurationUtil.isReadOnlyModeEnabled(conf)) { + if (!ConfigurationUtil.isReadOnlyModeEnabledInConf(conf)) { try { // verify the contents against the config set ActiveClusterSuffix acs = diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactSplit.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactSplit.java index 7b8de6b9d6e8..c233ce521e32 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactSplit.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactSplit.java @@ -346,7 +346,7 @@ protected void requestCompactionInternal(HRegion region, HStore store, String wh } // Should not allow compaction if cluster is in read-only mode - if (ConfigurationUtil.isReadOnlyModeEnabled(conf)) { + if (ConfigurationUtil.isReadOnlyModeEnabledInConf(conf)) { LOG.info("Ignoring compaction request for " + region + ",because read-only mode is on."); return; } @@ -451,7 +451,7 @@ private Optional selectCompaction(HRegion region, HStore stor } // Should not allow compaction if cluster is in read-only mode - if (ConfigurationUtil.isReadOnlyModeEnabled(conf)) { + if (ConfigurationUtil.isReadOnlyModeEnabledInConf(conf)) { LOG.info(String.format("Compaction request skipped as read-only mode is on")); return Optional.empty(); } 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 5d1246645a4f..36527a6d9343 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,8 +17,6 @@ */ 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; @@ -392,8 +390,6 @@ 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; @@ -944,8 +940,6 @@ public HRegion(final HRegionFileSystem fs, final WAL wal, final Configuration co decorateRegionConfiguration(conf); - this.isGlobalReadOnlyEnabled = - conf.getBoolean(HBASE_GLOBAL_READONLY_ENABLED_KEY, HBASE_GLOBAL_READONLY_ENABLED_DEFAULT); CoprocessorConfigurationUtil.syncReadOnlyConfigurations(this.conf, CoprocessorHost.REGION_COPROCESSOR_CONF_KEY); @@ -8993,11 +8987,16 @@ IOException throwOnInterrupt(Throwable t) { public void onConfigurationChange(Configuration newConf) { this.storeHotnessProtector.update(newConf); - CoprocessorConfigurationUtil.maybeUpdateCoprocessors(newConf, this.isGlobalReadOnlyEnabled, + boolean originalIsReadOnlyEnabled = CoprocessorConfigurationUtil + .areReadOnlyCoprocessorsLoaded(this.conf, CoprocessorHost.REGION_COPROCESSOR_CONF_KEY); + + CoprocessorConfigurationUtil.maybeUpdateCoprocessors(newConf, originalIsReadOnlyEnabled, this.coprocessorHost, CoprocessorHost.REGION_COPROCESSOR_CONF_KEY, false, this.toString(), - val -> this.isGlobalReadOnlyEnabled = val, conf -> { + conf -> { decorateRegionConfiguration(conf); - this.coprocessorHost = new RegionCoprocessorHost(this, rsServices, newConf); + this.coprocessorHost = new RegionCoprocessorHost(this, rsServices, conf); + CoprocessorConfigurationUtil.updateCoprocessorListInConf(this.conf, conf, + CoprocessorHost.REGION_COPROCESSOR_CONF_KEY); }); } @@ -9008,12 +9007,6 @@ public void onConfigurationChange(Configuration newConf) { public void registerChildren(ConfigurationManager manager) { configurationManager = manager; stores.values().forEach(manager::registerObserver); - if (coprocessorHost != null) { - coprocessorHost.registerConfigurationObservers(manager); - } else { - LOG.warn("Could not register HRegion coprocessors to the ConfigurationManager because " - + "RegionCoprocessorHost is null"); - } } /** @@ -9022,12 +9015,6 @@ public void registerChildren(ConfigurationManager manager) { @Override public void deregisterChildren(ConfigurationManager manager) { stores.values().forEach(configurationManager::deregisterObserver); - if (coprocessorHost != null) { - coprocessorHost.deregisterConfigurationObservers(manager); - } else { - LOG.warn("Could not deregister HRegion coprocessors from the ConfigurationManager because " - + "RegionCoprocessorHost is null"); - } } @Override 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 8cd6f738b339..f6bc6f3a71fc 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,8 +20,6 @@ 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; @@ -548,9 +546,6 @@ 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); @@ -2163,12 +2158,6 @@ private void registerConfigurationObservers() { configurationManager.registerObserver(this.rpcServices); configurationManager.registerObserver(this.prefetchExecutorNotifier); configurationManager.registerObserver(this); - if (rsHost != null) { - rsHost.registerConfigurationObservers(configurationManager); - } else { - LOG.warn("Could not register HRegionServer coprocessors to the ConfigurationManager because " - + "RegionServerCoprocessorHost is null"); - } } /* @@ -3493,10 +3482,16 @@ public void onConfigurationChange(Configuration newConf) { LOG.warn("Failed to initialize SuperUsers on reloading of the configuration"); } - CoprocessorConfigurationUtil.maybeUpdateCoprocessors(newConf, this.isGlobalReadOnlyEnabled, + boolean originalIsReadOnlyEnabled = CoprocessorConfigurationUtil + .areReadOnlyCoprocessorsLoaded(this.conf, CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY); + + CoprocessorConfigurationUtil.maybeUpdateCoprocessors(newConf, originalIsReadOnlyEnabled, this.rsHost, CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY, false, this.toString(), - val -> this.isGlobalReadOnlyEnabled = val, - conf -> this.rsHost = new RegionServerCoprocessorHost(this, newConf)); + conf -> { + this.rsHost = new RegionServerCoprocessorHost(this, conf); + CoprocessorConfigurationUtil.updateCoprocessorListInConf(this.conf, conf, + CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY); + }); } @Override diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/util/ConfigurationUtil.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/util/ConfigurationUtil.java index aed30e46af78..63e641f53643 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/util/ConfigurationUtil.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/util/ConfigurationUtil.java @@ -116,7 +116,7 @@ public static List> getKeyValues(Configuration conf, S return rtn; } - public static boolean isReadOnlyModeEnabled(Configuration conf) { + public static boolean isReadOnlyModeEnabledInConf(Configuration conf) { return conf.getBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT); } 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 ed0a8e8cb53e..234a35a80172 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,7 +22,6 @@ 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; @@ -51,8 +50,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. + * This method is useful when the configuration object has been updated, but we need to determine + * if the coprocessor configuration has actually changed compared to what's currently loaded. *

* Note: This method only detects changes in the set of coprocessor class names. It does * not detect changes to priority or path for coprocessors that are already loaded with the @@ -182,13 +181,14 @@ private static List getReadOnlyCoprocessors(String configurationKey) { /** * This method adds or removes relevant ReadOnlyController coprocessors to the provided - * configuration based on whether read-only mode is enabled. + * configuration based on whether read-only mode is enabled in the provided Configuration. * @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 = ConfigurationUtil.isReadOnlyModeEnabled(conf); + boolean isReadOnlyModeEnabled = conf.getBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, + HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT); List cpList = getReadOnlyCoprocessors(coprocessorConfKey); if (isReadOnlyModeEnabled) { @@ -198,6 +198,58 @@ public static void syncReadOnlyConfigurations(Configuration conf, String coproce } } + /** + * Check whether ReadOnlyController coprocessors have been loaded in the provided configuration. + * @param conf the configuration we are checking + * @param coprocessorConfKey configuration key used for setting master, region server, or region + * coprocessors + * @return true if the ReadOnlyCoprocessors are loaded in the configuration; false otherwise + */ + public static boolean areReadOnlyCoprocessorsLoaded(Configuration conf, + String coprocessorConfKey) { + // Using a HashSet will improve performance when searching for read-only coprocessors + HashSet allCoprocessors = + new HashSet<>(getCoprocessorsFromConfig(conf, coprocessorConfKey)); + List readOnlyCoprocessors = getReadOnlyCoprocessors(coprocessorConfKey); + return allCoprocessors.containsAll(readOnlyCoprocessors); + } + + /** + * Takes an updated configuration and updates the coprocessors for that configuration key in the + * current configuration. + * @param currentConf the configuration currently used by the master, region server, or + * region + * @param updatedConf the updated version of the configuration whose coprocessors we want + * to copy + * @param coprocessorConfKey configuration key used for setting master, region server, or region + * coprocessors + */ + public static void updateCoprocessorListInConf(Configuration currentConf, + Configuration updatedConf, String coprocessorConfKey) { + String[] updatedCoprocessorList = updatedConf.getStrings(coprocessorConfKey); + if (updatedCoprocessorList != null) { + currentConf.setStrings(coprocessorConfKey, updatedCoprocessorList); + } else { + currentConf.unset(coprocessorConfKey); + } + } + + /** + * Gets the name of a component based on the provided coprocessor configuration key. + * @param coprocessorConfKey configuration key used for setting master, region server, or region + * coprocessors + * @return the component type - Master, Region Server, or Region + */ + public static String getComponentName(String coprocessorConfKey) { + return switch (coprocessorConfKey) { + case CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY -> "Master"; + case CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY -> "Region Server"; + case CoprocessorHost.REGION_COPROCESSOR_CONF_KEY -> "Region"; + default -> throw new IllegalArgumentException( + "Unsupported coprocessor configuration key: " + coprocessorConfKey); + }; + } + /** * 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 @@ -212,32 +264,31 @@ public static void syncReadOnlyConfigurations(Configuration conf, String coproce * @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 stateSetter, CoprocessorReloadTask reloadTask) { + CoprocessorReloadTask reloadTask) { - boolean maybeUpdatedReadOnlyMode = ConfigurationUtil.isReadOnlyModeEnabled(newConf); + boolean maybeUpdatedReadOnlyMode = ConfigurationUtil.isReadOnlyModeEnabledInConf(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); + LOG.info("Updating coprocessors for {} {} because the configuration has changed", + getComponentName(coprocessorConfKey), 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); + LOG.info("Config {} has been dynamically changed to {} for {} {}", + HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, maybeUpdatedReadOnlyMode, + getComponentName(coprocessorConfKey), instance); } } } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestReadOnlyController.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestReadOnlyController.java index 506829681fc4..a94406e47264 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestReadOnlyController.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestReadOnlyController.java @@ -108,7 +108,7 @@ public void afterClass() throws Exception { private static void enableReadOnlyMode() { // Dynamically enable Read-Only mode if it is not active - if (!ConfigurationUtil.isReadOnlyModeEnabled(conf)) { + if (!ConfigurationUtil.isReadOnlyModeEnabledInConf(conf)) { LOG.info("Dynamically enabling Read-Only mode by setting {} to true", HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT); conf.setBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, true); @@ -118,7 +118,7 @@ private static void enableReadOnlyMode() { private static void disableReadOnlyMode() { // Dynamically disable Read-Only mode if it is active - if (ConfigurationUtil.isReadOnlyModeEnabled(conf)) { + if (ConfigurationUtil.isReadOnlyModeEnabledInConf(conf)) { LOG.info("Dynamically disabling Read-Only mode by setting {} to false", HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT); conf.setBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, false); 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 ab63f719c8bf..2e7906255b5a 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 @@ -111,6 +111,9 @@ private Configuration setReadOnlyMode(boolean isReadOnlyEnabled) { newConf.setBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, isReadOnlyEnabled); master.getConfigurationManager().notifyAllObservers(newConf); regionServer.getConfigurationManager().notifyAllObservers(newConf); + if (region != null) { + region.getConfigurationManager().notifyAllObservers(newConf); + } return newConf; } From a70b99132f1c92dfcd82aca8498bae54a3086936 Mon Sep 17 00:00:00 2001 From: Kevin Geiszler Date: Fri, 24 Apr 2026 09:23:21 -0700 Subject: [PATCH 2/4] Make ReadOnlyController coprocessor lists static; Use isReadOnlyModeEnabledInConf() Change-Id: Ic4983542901998cfa7e6fca1c48137c84405ce2f --- .../util/CoprocessorConfigurationUtil.java | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) 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 234a35a80172..f1000481bdb0 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 @@ -45,6 +45,14 @@ public final class CoprocessorConfigurationUtil { private static final Logger LOG = LoggerFactory.getLogger(CoprocessorConfigurationUtil.class); + public static List MASTER_READONLY_CONTROLLER_COPROCESSORS = + List.of(MasterReadOnlyController.class.getName()); + public static List REGIONSERVER_READONLY_CONTROLLER_COPROCESSORS = + List.of(RegionServerReadOnlyController.class.getName()); + public static List REGION_READONLY_CONTROLLER_COPROCESSORS = + List.of(RegionReadOnlyController.class.getName(), BulkLoadReadOnlyController.class.getName(), + EndpointReadOnlyController.class.getName()); + private CoprocessorConfigurationUtil() { } @@ -164,16 +172,9 @@ public static void removeCoprocessors(Configuration conf, String configurationKe private static List getReadOnlyCoprocessors(String configurationKey) { return switch (configurationKey) { - case CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY -> List - .of(MasterReadOnlyController.class.getName()); - - case CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY -> List - .of(RegionServerReadOnlyController.class.getName()); - - case CoprocessorHost.REGION_COPROCESSOR_CONF_KEY -> List.of( - RegionReadOnlyController.class.getName(), BulkLoadReadOnlyController.class.getName(), - EndpointReadOnlyController.class.getName()); - + case CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY -> MASTER_READONLY_CONTROLLER_COPROCESSORS; + case CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY -> REGIONSERVER_READONLY_CONTROLLER_COPROCESSORS; + case CoprocessorHost.REGION_COPROCESSOR_CONF_KEY -> REGION_READONLY_CONTROLLER_COPROCESSORS; default -> throw new IllegalArgumentException( "Unsupported coprocessor configuration key: " + configurationKey); }; @@ -187,8 +188,7 @@ private static List getReadOnlyCoprocessors(String configurationKey) { * @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); + boolean isReadOnlyModeEnabled = ConfigurationUtil.isReadOnlyModeEnabledInConf(conf); List cpList = getReadOnlyCoprocessors(coprocessorConfKey); if (isReadOnlyModeEnabled) { From 83f2e0aee4d375be336c798d42444057f34e0f82 Mon Sep 17 00:00:00 2001 From: Kevin Geiszler Date: Fri, 24 Apr 2026 14:55:26 -0700 Subject: [PATCH 3/4] Use ImmutableMap and ImmutableList Change-Id: Ib7185376610a02e28f15d9e9858355c8d8b99378 --- .../util/CoprocessorConfigurationUtil.java | 25 ++++++++----------- 1 file changed, 11 insertions(+), 14 deletions(-) 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 f1000481bdb0..3f8c03648088 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 @@ -37,6 +37,8 @@ import org.apache.hbase.thirdparty.com.google.common.base.Preconditions; import org.apache.hbase.thirdparty.com.google.common.base.Strings; +import org.apache.hbase.thirdparty.com.google.common.collect.ImmutableList; +import org.apache.hbase.thirdparty.com.google.common.collect.ImmutableMap; /** * Helper class for coprocessor host when configuration changes. @@ -45,13 +47,14 @@ public final class CoprocessorConfigurationUtil { private static final Logger LOG = LoggerFactory.getLogger(CoprocessorConfigurationUtil.class); - public static List MASTER_READONLY_CONTROLLER_COPROCESSORS = - List.of(MasterReadOnlyController.class.getName()); - public static List REGIONSERVER_READONLY_CONTROLLER_COPROCESSORS = - List.of(RegionServerReadOnlyController.class.getName()); - public static List REGION_READONLY_CONTROLLER_COPROCESSORS = - List.of(RegionReadOnlyController.class.getName(), BulkLoadReadOnlyController.class.getName(), - EndpointReadOnlyController.class.getName()); + public static ImmutableMap> READONLY_COPROCESSORS = + ImmutableMap.of(CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY, + ImmutableList.of(MasterReadOnlyController.class.getName()), + CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY, + ImmutableList.of(RegionServerReadOnlyController.class.getName()), + CoprocessorHost.REGION_COPROCESSOR_CONF_KEY, + ImmutableList.of(RegionReadOnlyController.class.getName(), + BulkLoadReadOnlyController.class.getName(), EndpointReadOnlyController.class.getName())); private CoprocessorConfigurationUtil() { } @@ -171,13 +174,7 @@ public static void removeCoprocessors(Configuration conf, String configurationKe } private static List getReadOnlyCoprocessors(String configurationKey) { - return switch (configurationKey) { - case CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY -> MASTER_READONLY_CONTROLLER_COPROCESSORS; - case CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY -> REGIONSERVER_READONLY_CONTROLLER_COPROCESSORS; - case CoprocessorHost.REGION_COPROCESSOR_CONF_KEY -> REGION_READONLY_CONTROLLER_COPROCESSORS; - default -> throw new IllegalArgumentException( - "Unsupported coprocessor configuration key: " + configurationKey); - }; + return READONLY_COPROCESSORS.get(configurationKey); } /** From 7320fa7151e7ef5731d4c2c6a6c3410f01957e6b Mon Sep 17 00:00:00 2001 From: Kevin Geiszler Date: Fri, 24 Apr 2026 15:59:02 -0700 Subject: [PATCH 4/4] Make static READONLY_COPROCESSORS private and final Change-Id: Ie60a3584b7f4bde601bd760807c510984099c811 --- .../apache/hadoop/hbase/util/CoprocessorConfigurationUtil.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 3f8c03648088..8c462fbbc348 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 @@ -47,7 +47,7 @@ public final class CoprocessorConfigurationUtil { private static final Logger LOG = LoggerFactory.getLogger(CoprocessorConfigurationUtil.class); - public static ImmutableMap> READONLY_COPROCESSORS = + private static final ImmutableMap> READONLY_COPROCESSORS = ImmutableMap.of(CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY, ImmutableList.of(MasterReadOnlyController.class.getName()), CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY,