Skip to content

HBASE-30180: Can still add data to read-only region after flipping read-only flag multiple times #8280

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kgeisz wants to merge 1 commit into
base: master
Choose a base branch
from
+170 −64
Open

HBASE-30180: Can still add data to read-only region after flipping read-only flag multiple times #8280

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 16 additions & 10 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4491,26 +4491,32 @@ public MasterRegion getMasterRegion() {
return masterRegion;
}

/**
* Dynamically updates HMaster's configuration. Since HMaster inherits from
* {@link HBaseServerBase}, the {@code updatedConf} parameter references the same
* {@link Configuration} object as HMaster's {@code this.conf} instance variable in a real HBase
* deployment. This isn't necessarily the case in unit tests.
* @param updatedConf the dynamically updated configuration
*/
@Override
public void onConfigurationChange(Configuration newConf) {
public void onConfigurationChange(Configuration updatedConf) {
try {
Superusers.initialize(newConf);
Superusers.initialize(updatedConf);
} catch (IOException e) {
LOG.warn("Failed to initialize SuperUsers on reloading of the configuration");
}
// append the quotas observer back to the master coprocessor key
setQuotasObserver(newConf);
setQuotasObserver(updatedConf);

boolean originalIsReadOnlyEnabled = CoprocessorConfigurationUtil
.areReadOnlyCoprocessorsLoaded(this.conf, CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY);

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);
});
// updatedConf and this.conf reference the same Configuration object in an actual HBase
// deployment. However, in unit test cases they reference different Configuration objects, so
// this.conf needs to be updated.
CoprocessorConfigurationUtil.maybeUpdateCoprocessors(updatedConf, this.conf,
originalIsReadOnlyEnabled, this.cpHost, CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY,
this.maintenanceMode, this.toString(), this::initializeCoprocessorHost);

boolean maybeUpdatedReadOnlyMode = CoprocessorConfigurationUtil
.areReadOnlyCoprocessorsLoaded(this.conf, CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY);
Expand Down
33 changes: 24 additions & 9 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@
import org.apache.hadoop.hbase.keymeta.KeyManagementService;
import org.apache.hadoop.hbase.keymeta.ManagedKeyDataCache;
import org.apache.hadoop.hbase.keymeta.SystemKeyCache;
import org.apache.hadoop.hbase.master.HMaster;
import org.apache.hadoop.hbase.mob.MobFileCache;
import org.apache.hadoop.hbase.monitoring.MonitoredTask;
import org.apache.hadoop.hbase.monitoring.TaskMonitor;
Expand Down Expand Up @@ -8982,25 +8983,33 @@ IOException throwOnInterrupt(Throwable t) {
}

/**
* {@inheritDoc}
* Dynamically updates HRegion's configuration. Unlike {@link HMaster} and {@link HRegionServer},
* this {@code updatedConf} parameter does not reference the same {@link Configuration} object as
* HRegion's {@code this.conf} instance variable in a real HBase deployment. This is because
* HRegion's {@code this.conf} is a {@link CompoundConfiguration} object. Instead,
* {@code updatedConf} references the same Configuration object as HRegion's {@code this.baseConf}
* instance variable.
* @param updatedConf the dynamically updated configuration
*/
@Override
public void onConfigurationChange(Configuration newConf) {
this.storeHotnessProtector.update(newConf);
public void onConfigurationChange(Configuration updatedConf) {
this.storeHotnessProtector.update(updatedConf);

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 updatedConf 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.
CoprocessorConfigurationUtil.maybeUpdateCoprocessors(updatedConf, 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);
boolean newReadOnlyEnabled = ConfigurationUtil.isReadOnlyModeEnabledInConf(updatedConf);

if (originalIsReadOnlyEnabled && !newReadOnlyEnabled) {
LOG.info("Cluster Read Only mode disabled");
Expand Down Expand Up @@ -9157,4 +9166,10 @@ boolean isReadsEnabled() {
public ConfigurationManager getConfigurationManager() {
return configurationManager;
}

@RestrictedApi(explanation = "Should only be called in tests", link = "",
allowedOnPath = ".*/src/test/.*")
public Configuration getConfiguration() {
return this.conf;
}
}
26 changes: 16 additions & 10 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3483,29 +3483,35 @@ public double getFlushPressure() {
return getRegionServerAccounting().getFlushPressure();
}

/**
* Dynamically updates HRegionServer's configuration. Since HRegionServer inherits from
* {@link HBaseServerBase}, the {@code updatedConf} parameter references the same
* {@link Configuration} object as HRegionServer's {@code this.conf} instance variable in a real
* HBase deployment. This isn't necessarily the case in unit tests.
* @param updatedConf the dynamically updated configuration
*/
@Override
public void onConfigurationChange(Configuration newConf) {
public void onConfigurationChange(Configuration updatedConf) {
ThroughputController old = this.flushThroughputController;
if (old != null) {
old.stop("configuration change");
}
this.flushThroughputController = FlushThroughputControllerFactory.create(this, newConf);
this.flushThroughputController = FlushThroughputControllerFactory.create(this, updatedConf);
try {
Superusers.initialize(newConf);
Superusers.initialize(updatedConf);
} catch (IOException e) {
LOG.warn("Failed to initialize SuperUsers on reloading of the configuration");
}

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(),
conf -> {
this.rsHost = new RegionServerCoprocessorHost(this, conf);
CoprocessorConfigurationUtil.updateCoprocessorListInConf(this.conf, conf,
CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY);
});
// updatedConf and this.conf reference the same Configuration object in an actual HBase
// deployment. However, in unit test cases they reference different Configuration objects, so
// this.conf needs to be updated.
CoprocessorConfigurationUtil.maybeUpdateCoprocessors(updatedConf, this.conf,
originalIsReadOnlyEnabled, this.rsHost, CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY,
false, this.toString(), conf -> this.rsHost = new RegionServerCoprocessorHost(this, conf));
}

@Override
Expand Down
74 changes: 43 additions & 31 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/util/CoprocessorConfigurationUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,13 @@
*/
package org.apache.hadoop.hbase.util;

import static org.apache.hadoop.hbase.HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashSet;
import java.util.List;
import java.util.Objects;
import java.util.Set;
import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.hbase.HConstants;
Expand Down Expand Up @@ -177,6 +180,25 @@ private static List<String> getReadOnlyCoprocessors(String configurationKey) {
return READONLY_COPROCESSORS.get(configurationKey);
}

/**
* Updates the coprocessors in one Configuration object to match the coprocessors in the other
* @param srcConf the Configuration object we are getting coprocessors from
* @param dstConf the Configuration object we are updating
* @param coprocessorConfKey the type of coprocessors we are updating
*/
private static void syncCoprocessorsWithConf(Configuration srcConf, Configuration dstConf,
String coprocessorConfKey) {
String configuredCps = srcConf.get(coprocessorConfKey);
dstConf.set(coprocessorConfKey, Objects.requireNonNullElse(configuredCps, ""));

// A configuration with region coprocessors may also have user region coprocessors
if (CoprocessorHost.REGION_COPROCESSOR_CONF_KEY.equals(coprocessorConfKey)) {
String configuredUserCps = srcConf.get(CoprocessorHost.USER_REGION_COPROCESSOR_CONF_KEY);
dstConf.set(CoprocessorHost.USER_REGION_COPROCESSOR_CONF_KEY,
Objects.requireNonNullElse(configuredUserCps, ""));
}
}

/**
* This method adds or removes relevant ReadOnlyController coprocessors to the provided
* configuration based on whether read-only mode is enabled in the provided Configuration.
Expand Down Expand Up @@ -211,26 +233,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
Expand All @@ -252,7 +254,9 @@ public static String getComponentName(String coprocessorConfKey) {
* 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 updatedConf an updated configuration
* @param originalConf the actual configuration we want to update, which may or may
* not be the same Configuration object as {@code updatedConf}
* @param originalIsReadOnlyEnabled the original value for
* {@value HConstants#HBASE_GLOBAL_READONLY_ENABLED_KEY}
* @param coprocessorHost the coprocessor host for HMaster, HRegionServer, or HRegion
Expand All @@ -264,28 +268,36 @@ 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 updatedConf, Configuration originalConf,
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 updatedReadOnlyMode = ConfigurationUtil.isReadOnlyModeEnabledInConf(updatedConf);
boolean hasReadOnlyModeChanged = originalIsReadOnlyEnabled != updatedReadOnlyMode;
boolean hasCoprocessorConfigChanged = CoprocessorConfigurationUtil
.checkConfigurationChange(coprocessorHost, newConf, coprocessorConfKey);
.checkConfigurationChange(coprocessorHost, updatedConf, 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);
// In real HBase deployments, updatedConf and originalConf reference the same Configuration
// object for HMaster and HRegionServer respectively. However, for HRegion and for unit test
// cases, these are different objects, and the original conf needs to be updated accordingly.
if (updatedConf != originalConf) {
syncCoprocessorsWithConf(updatedConf, originalConf, coprocessorConfKey);
originalConf.setBoolean(HBASE_GLOBAL_READONLY_ENABLED_KEY, updatedReadOnlyMode);
}
// This needs to run even if read-only mode has not changed in case ReadOnly coprocessors
// were unintentionally added/removed in the previous code block
syncReadOnlyConfigurations(originalConf, coprocessorConfKey);
reloadTask.reload(originalConf);
}

if (hasReadOnlyModeChanged) {
LOG.info("Config {} has been dynamically changed to {} for {} {}",
HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, maybeUpdatedReadOnlyMode,
getComponentName(coprocessorConfKey), instance);
HBASE_GLOBAL_READONLY_ENABLED_KEY, updatedReadOnlyMode, componentName, instance);
}
}
}
Loading