From 024d94c69de745a374807d4f7ebcf16036783f7f Mon Sep 17 00:00:00 2001 From: Kevin Geiszler Date: Wed, 27 May 2026 15:32:08 -0700 Subject: [PATCH 1/2] HBASE-30180: Can still add data to read-only region after flipping read-only flag multiple times Change-Id: I8c5edf6ae4775d36dbbe2b260f77e2168a2a92da --- .../apache/hadoop/hbase/master/HMaster.java | 8 +-- .../hadoop/hbase/regionserver/HRegion.java | 14 +++-- .../hbase/regionserver/HRegionServer.java | 8 +-- .../util/CoprocessorConfigurationUtil.java | 55 +++++++++---------- 4 files changed, 41 insertions(+), 44 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 df6c616db4ae..71b142bc0c49 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 @@ -4504,13 +4504,11 @@ public void onConfigurationChange(Configuration newConf) { boolean originalIsReadOnlyEnabled = CoprocessorConfigurationUtil .areReadOnlyCoprocessorsLoaded(this.conf, CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY); + // newConf and this.conf reference the same Configuration object, so it doesn't matter which + // one we update CoprocessorConfigurationUtil.maybeUpdateCoprocessors(newConf, originalIsReadOnlyEnabled, this.cpHost, CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY, this.maintenanceMode, - this.toString(), conf -> { - this.initializeCoprocessorHost(conf); - CoprocessorConfigurationUtil.updateCoprocessorListInConf(this.conf, conf, - CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY); - }); + this.toString(), this::initializeCoprocessorHost); boolean maybeUpdatedReadOnlyMode = CoprocessorConfigurationUtil .areReadOnlyCoprocessorsLoaded(this.conf, CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY); 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 f2a67df842ce..db8000db9d24 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 @@ -8991,13 +8991,17 @@ public void onConfigurationChange(Configuration newConf) { 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(), - conf -> { + // HRegion's this.conf is a special Configuration type called CompoundConfiguration. This means + // we don't want to use the newConf provided in onConfigurationChange() for creating a new + // RegionCoprocessorHost. Instead, we update this.conf and use that for decorating the region + // config and updating this.coprocessorHost. Also, we do not need to explicitly update + // hbase.global.readonly.enabled in this.conf because the CompoundConfiguration holds a + // reference to HBaseServerBase's conf, which was updated automatically. + CoprocessorConfigurationUtil.maybeUpdateCoprocessors(newConf, this.conf, + originalIsReadOnlyEnabled, this.coprocessorHost, CoprocessorHost.REGION_COPROCESSOR_CONF_KEY, + false, this.toString(), conf -> { decorateRegionConfiguration(conf); this.coprocessorHost = new RegionCoprocessorHost(this, rsServices, conf); - CoprocessorConfigurationUtil.updateCoprocessorListInConf(this.conf, conf, - CoprocessorHost.REGION_COPROCESSOR_CONF_KEY); }); boolean newReadOnlyEnabled = ConfigurationUtil.isReadOnlyModeEnabledInConf(newConf); 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 009e0d8a1341..cf485ac0c02f 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 @@ -3499,13 +3499,11 @@ public void onConfigurationChange(Configuration newConf) { boolean originalIsReadOnlyEnabled = CoprocessorConfigurationUtil .areReadOnlyCoprocessorsLoaded(this.conf, CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY); + // newConf and this.conf reference the same Configuration object, so it doesn't matter which + // one we update CoprocessorConfigurationUtil.maybeUpdateCoprocessors(newConf, originalIsReadOnlyEnabled, this.rsHost, CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY, false, this.toString(), - conf -> { - this.rsHost = new RegionServerCoprocessorHost(this, conf); - CoprocessorConfigurationUtil.updateCoprocessorListInConf(this.conf, conf, - CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY); - }); + conf -> this.rsHost = new RegionServerCoprocessorHost(this, conf)); } @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 8c462fbbc348..e5b2c01222ae 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 @@ -211,26 +211,6 @@ public static boolean areReadOnlyCoprocessorsLoaded(Configuration conf, 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 @@ -253,6 +233,8 @@ public static String getComponentName(String coprocessorConfKey) { * 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 confToUpdate the actual configuration we want to update, which may or may + * not be the same as {@code newConf} * @param originalIsReadOnlyEnabled the original value for * {@value HConstants#HBASE_GLOBAL_READONLY_ENABLED_KEY} * @param coprocessorHost the coprocessor host for HMaster, HRegionServer, or HRegion @@ -264,28 +246,43 @@ public static String getComponentName(String coprocessorConfKey) { * @param reloadTask lambda function that reloads coprocessors on the master, * region server, or region */ - public static void maybeUpdateCoprocessors(Configuration newConf, + public static void maybeUpdateCoprocessors(Configuration newConf, Configuration confToUpdate, boolean originalIsReadOnlyEnabled, CoprocessorHost coprocessorHost, String coprocessorConfKey, boolean isMaintenanceMode, String instance, CoprocessorReloadTask reloadTask) { - boolean maybeUpdatedReadOnlyMode = ConfigurationUtil.isReadOnlyModeEnabledInConf(newConf); - boolean hasReadOnlyModeChanged = originalIsReadOnlyEnabled != maybeUpdatedReadOnlyMode; + String componentName = getComponentName(coprocessorConfKey); + boolean currentReadOnlyMode = ConfigurationUtil.isReadOnlyModeEnabledInConf(newConf); + boolean hasReadOnlyModeChanged = originalIsReadOnlyEnabled != currentReadOnlyMode; 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", - getComponentName(coprocessorConfKey), instance); - CoprocessorConfigurationUtil.syncReadOnlyConfigurations(newConf, coprocessorConfKey); - reloadTask.reload(newConf); + componentName, instance); + if (hasReadOnlyModeChanged) { + CoprocessorConfigurationUtil.syncReadOnlyConfigurations(confToUpdate, coprocessorConfKey); + } + reloadTask.reload(confToUpdate); } if (hasReadOnlyModeChanged) { LOG.info("Config {} has been dynamically changed to {} for {} {}", - HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, maybeUpdatedReadOnlyMode, - getComponentName(coprocessorConfKey), instance); + HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, currentReadOnlyMode, componentName, instance); } } + + /** + * Similar to the other implementation of maybeUpdateCoprocessors(), except newConf is the conf + * being updated. This is useful for onConfigurationChange() in HMaster and HRegionServer, where + * their this.conf variable references the same object as the newConf arg in their respective + * onConfigurationChange() method. + */ + public static void maybeUpdateCoprocessors(Configuration newConf, + boolean originalIsReadOnlyEnabled, CoprocessorHost coprocessorHost, + String coprocessorConfKey, boolean isMaintenanceMode, String instance, + CoprocessorReloadTask reloadTask) { + maybeUpdateCoprocessors(newConf, newConf, originalIsReadOnlyEnabled, coprocessorHost, + coprocessorConfKey, isMaintenanceMode, instance, reloadTask); + } } From 889977e68abefd90e6ae8d448946b1b564974602 Mon Sep 17 00:00:00 2001 From: Kevin Geiszler Date: Sat, 30 May 2026 15:10:23 -0700 Subject: [PATCH 2/2] Fix unit tests; add new unit test case Change-Id: I11accf45b4e92184f289652bba9a32e7651b5af7 --- .../apache/hadoop/hbase/master/HMaster.java | 2 +- .../hadoop/hbase/regionserver/HRegion.java | 13 ++++-- .../hbase/regionserver/HRegionServer.java | 2 +- .../util/CoprocessorConfigurationUtil.java | 4 +- .../hbase/regionserver/TestHRegion.java | 45 +++++++++++++++++-- ...tReadOnlyControllerCoprocessorLoading.java | 3 +- 6 files changed, 57 insertions(+), 12 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 71b142bc0c49..b668e53e10db 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 @@ -4506,7 +4506,7 @@ public void onConfigurationChange(Configuration newConf) { // newConf and this.conf reference the same Configuration object, so it doesn't matter which // one we update - CoprocessorConfigurationUtil.maybeUpdateCoprocessors(newConf, originalIsReadOnlyEnabled, + CoprocessorConfigurationUtil.maybeUpdateCoprocessors(newConf, this.conf, originalIsReadOnlyEnabled, this.cpHost, CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY, this.maintenanceMode, this.toString(), this::initializeCoprocessorHost); 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 db8000db9d24..eb5049b72e75 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 @@ -383,7 +383,7 @@ public class HRegion implements HeapSize, PropagatingConfigurationObserver, Regi private MobFileCache mobFileCache; private final WAL wal; private final HRegionFileSystem fs; - protected final Configuration conf; + protected Configuration conf; private final Configuration baseConf; private final int rowLockWaitDuration; static final int DEFAULT_ROWLOCK_WAIT_DURATION = 30000; @@ -492,6 +492,10 @@ public long getSmallestReadPoint() { } } + public Configuration getConfiguration() { + return this.conf; + } + /* * Data structure of write state flags used coordinating flushes, compactions and closes. */ @@ -8997,11 +9001,12 @@ public void onConfigurationChange(Configuration newConf) { // config and updating this.coprocessorHost. Also, we do not need to explicitly update // hbase.global.readonly.enabled in this.conf because the CompoundConfiguration holds a // reference to HBaseServerBase's conf, which was updated automatically. - CoprocessorConfigurationUtil.maybeUpdateCoprocessors(newConf, this.conf, + CoprocessorConfigurationUtil.maybeUpdateCoprocessors(newConf, originalIsReadOnlyEnabled, this.coprocessorHost, CoprocessorHost.REGION_COPROCESSOR_CONF_KEY, false, this.toString(), conf -> { - decorateRegionConfiguration(conf); - this.coprocessorHost = new RegionCoprocessorHost(this, rsServices, conf); + this.conf = new CompoundConfiguration().add(conf).addBytesMap(this.htableDescriptor.getValues()); + decorateRegionConfiguration(this.conf); + this.coprocessorHost = new RegionCoprocessorHost(this, rsServices, this.conf); }); boolean newReadOnlyEnabled = ConfigurationUtil.isReadOnlyModeEnabledInConf(newConf); 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 cf485ac0c02f..1b04866232c6 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 @@ -3501,7 +3501,7 @@ public void onConfigurationChange(Configuration newConf) { // newConf and this.conf reference the same Configuration object, so it doesn't matter which // one we update - CoprocessorConfigurationUtil.maybeUpdateCoprocessors(newConf, originalIsReadOnlyEnabled, + CoprocessorConfigurationUtil.maybeUpdateCoprocessors(newConf, this.conf, originalIsReadOnlyEnabled, this.rsHost, CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY, false, this.toString(), conf -> this.rsHost = new RegionServerCoprocessorHost(this, conf)); } 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 e5b2c01222ae..534858f8da87 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 @@ -39,6 +39,7 @@ 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; +import static org.apache.hadoop.hbase.HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY; /** * Helper class for coprocessor host when configuration changes. @@ -261,6 +262,7 @@ public static void maybeUpdateCoprocessors(Configuration newConf, Configuration LOG.info("Updating coprocessors for {} {} because the configuration has changed", componentName, instance); if (hasReadOnlyModeChanged) { + confToUpdate.setBoolean(HBASE_GLOBAL_READONLY_ENABLED_KEY, currentReadOnlyMode); CoprocessorConfigurationUtil.syncReadOnlyConfigurations(confToUpdate, coprocessorConfKey); } reloadTask.reload(confToUpdate); @@ -268,7 +270,7 @@ public static void maybeUpdateCoprocessors(Configuration newConf, Configuration if (hasReadOnlyModeChanged) { LOG.info("Config {} has been dynamically changed to {} for {} {}", - HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, currentReadOnlyMode, componentName, instance); + HBASE_GLOBAL_READONLY_ENABLED_KEY, currentReadOnlyMode, componentName, instance); } } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java index c138b0a448a7..d072dd630cbe 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java @@ -159,11 +159,13 @@ import org.apache.hadoop.hbase.regionserver.wal.WALUtil; import org.apache.hadoop.hbase.replication.regionserver.ReplicationObserver; import org.apache.hadoop.hbase.security.User; +import org.apache.hadoop.hbase.security.access.RegionReadOnlyController; import org.apache.hadoop.hbase.test.MetricsAssertHelper; import org.apache.hadoop.hbase.testclassification.LargeTests; import org.apache.hadoop.hbase.testclassification.VerySlowRegionServerTests; import org.apache.hadoop.hbase.util.Bytes; import org.apache.hadoop.hbase.util.CommonFSUtils; +import org.apache.hadoop.hbase.util.CoprocessorConfigurationUtil; import org.apache.hadoop.hbase.util.EnvironmentEdgeManager; import org.apache.hadoop.hbase.util.EnvironmentEdgeManagerTestHelper; import org.apache.hadoop.hbase.util.HFileArchiveUtil; @@ -7891,9 +7893,9 @@ public void testRegionOnCoprocessorsChange() throws IOException { NoOpRegionCoprocessor.class.getName()); // trigger configuration change region.onConfigurationChange(newConf); - assertTrue(region.getCoprocessorHost() != null); + assertNotNull(region.getCoprocessorHost()); Set coprocessors = region.getCoprocessorHost().getCoprocessors(); - assertTrue(coprocessors.size() == 2); + assertEquals(2, coprocessors.size()); assertTrue(region.getCoprocessorHost().getCoprocessors() .contains(MetaTableMetrics.class.getSimpleName())); assertTrue(region.getCoprocessorHost().getCoprocessors() @@ -7902,9 +7904,9 @@ public void testRegionOnCoprocessorsChange() throws IOException { // remove region coprocessor and keep only user region coprocessor newConf.unset(CoprocessorHost.REGION_COPROCESSOR_CONF_KEY); region.onConfigurationChange(newConf); - assertTrue(region.getCoprocessorHost() != null); + assertNotNull(region.getCoprocessorHost()); coprocessors = region.getCoprocessorHost().getCoprocessors(); - assertTrue(coprocessors.size() == 1); + assertEquals(1, coprocessors.size()); assertTrue(region.getCoprocessorHost().getCoprocessors() .contains(NoOpRegionCoprocessor.class.getSimpleName())); } @@ -8006,4 +8008,39 @@ public void testHRegionInitializeFailsWithDeletedRegionDir() throws Exception { TEST_UTIL.cleanupTestDir(); } } + + /** + * Regression for HBASE-30180: a region can miss read-only coprocessors after repeated toggles if + * {@code maybeUpdateCoprocessors} syncs to the server configuration instead of the region's + * {@link org.apache.hadoop.hbase.CompoundConfiguration}. + */ + @Test + public void testRegionOnConfigurationChangeLoadsReadOnlyCoprocessorsAfterRepeatedToggle() + throws IOException { + byte[] cf1 = Bytes.toBytes("CF1"); + Configuration serverConf = new Configuration(CONF); + serverConf.setBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, false); + + region = initHRegion(tableName, method, serverConf, new byte[][] { cf1 }); + region.setCoprocessorHost(new RegionCoprocessorHost(region, null, region.getConfiguration())); + + String regionCpKey = CoprocessorHost.REGION_COPROCESSOR_CONF_KEY; + boolean[] toggleSequence = new boolean[] { true, false, true, true, false }; + + for (boolean readOnlyEnabled : toggleSequence) { + Configuration newConf = new Configuration(serverConf); + newConf.setBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, readOnlyEnabled); + region.onConfigurationChange(newConf); + + if (readOnlyEnabled) { + assertNotNull( + region.getCoprocessorHost().findCoprocessor(RegionReadOnlyController.class.getName())); + } else { + assertNull( + region.getCoprocessorHost().findCoprocessor(RegionReadOnlyController.class.getName())); + } + assertEquals(readOnlyEnabled, CoprocessorConfigurationUtil + .areReadOnlyCoprocessorsLoaded(region.getConfiguration(), regionCpKey)); + } + } } 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 2e7906255b5a..44dc51573167 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 @@ -17,6 +17,7 @@ */ package org.apache.hadoop.hbase.security.access; +import static org.apache.hadoop.hbase.HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; @@ -81,7 +82,7 @@ public void tearDown() throws Exception { private void setupMiniCluster(boolean isReadOnlyEnabled) throws Exception { conf = TEST_UTIL.getConfiguration(); - conf.setBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, isReadOnlyEnabled); + conf.setBoolean(HBASE_GLOBAL_READONLY_ENABLED_KEY, isReadOnlyEnabled); TEST_UTIL.startMiniCluster(1); master = TEST_UTIL.getMiniHBaseCluster().getMaster();