taklwu commented on code in PR #8377:
URL: https://github.com/apache/hbase/pull/8377#discussion_r3581945527
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestReadOnlyManageActiveClusterFile.java:
##########
@@ -146,4 +166,176 @@ public void
testDoNotDeleteActiveClusterIdFileWhenSwitchingToReadOnlyIfNotOwnedB
// switching to readonly mode
assertTrue(activeClusterIdFileExists());
}
+
+ @Test
+ public void testCannotDisableReadOnlyWhenAnotherClusterIsActive() throws
Exception {
+ // First enable read-only mode (simulating a replica cluster)
+ setReadOnlyMode(true);
+ assertFalse(activeClusterIdFileExists());
+
+ // Now write an active cluster file with a DIFFERENT cluster's data
(simulating another active
+ // cluster owning the storage)
+ overwriteExistingFile();
+ assertTrue(activeClusterIdFileExists());
+
+ // Attempt to disable read-only mode, but get an exception because another
active cluster
+ // already exists.
+
master.getConfiguration().setBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY,
false);
+
+ // Master's updateConfiguration should throw because another cluster is
active
+ assertThrows(ReadOnlyTransitionException.class, () ->
master.updateConfiguration());
+
+ // Verify read-only coprocessors are still loaded on the master
+
assertTrue(CoprocessorConfigurationUtil.areReadOnlyCoprocessorsLoaded(master.getConfiguration(),
+ CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY));
+
+ // Verify read-only coprocessors are still loaded on the region server
+ assertTrue(CoprocessorConfigurationUtil.areReadOnlyCoprocessorsLoaded(
+ regionServer.getConfiguration(),
CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY));
+ }
+
+ @Test
+ public void testCanDisableReadOnlyWhenOwnClusterIsActive() throws Exception {
+ // Enable read-only mode
+ setReadOnlyMode(true);
+ assertFalse(activeClusterIdFileExists());
+
+ // Verify read-only coprocessors are loaded
+
assertTrue(CoprocessorConfigurationUtil.areReadOnlyCoprocessorsLoaded(master.getConfiguration(),
+ CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY));
+
+ // Disable read-only mode (our own cluster, no conflicting active cluster
file)
+ setReadOnlyMode(false);
+
+ // Active cluster file should be recreated
+ assertTrue(activeClusterIdFileExists());
+
+ // Verify read-only coprocessors are removed
+ assertFalse(CoprocessorConfigurationUtil.areReadOnlyCoprocessorsLoaded(
+ master.getConfiguration(), CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY));
+ }
+
+ @Test
+ public void testRegionCoprocessorsStillLoadWhenReadOnlyTransitionBlocked()
throws Exception {
+ // Create a table so we have a region to work with
+ TableName tableName = TableName.valueOf("testCoprocessorLoadTable");
+ TableDescriptor desc = TableDescriptorBuilder.newBuilder(tableName)
+ .setColumnFamily(ColumnFamilyDescriptorBuilder.of("cf")).build();
+ TEST_UTIL.getAdmin().createTable(desc);
+ List<HRegion> regions = regionServer.getRegions(tableName);
+ assertFalse(regions.isEmpty());
+ HRegion region = regions.get(0);
+
+ // Enable read-only mode to load ReadOnly coprocessors on the region
+ Configuration readOnlyConf = new Configuration(conf);
+ readOnlyConf.setBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY,
true);
+ region.onConfigurationChange(readOnlyConf);
+
+ // Verify ReadOnly coprocessors are loaded
+ RegionCoprocessorHost regionCPHost = region.getCoprocessorHost();
+
assertNotNull(regionCPHost.findCoprocessor(RegionReadOnlyController.class.getName()),
+ "RegionReadOnlyController should be loaded after enabling read-only
mode");
+
+ // Simulate another active cluster by writing a foreign cluster ID to the
active cluster file
+ overwriteExistingFile();
+ assertTrue(activeClusterIdFileExists());
+
+ // Build a config that attempts to disable read-only AND adds new
coprocessors
+ Configuration newConf = new Configuration(conf);
+ newConf.setBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, false);
+
+ // Add a system and user region coprocessors
+ newConf.set(CoprocessorHost.REGION_COPROCESSOR_CONF_KEY,
SimpleRegionObserver.class.getName());
+ newConf.set(CoprocessorHost.USER_REGION_COPROCESSOR_CONF_KEY,
+ NoOpScanPolicyObserver.class.getName());
+
+ // Trigger dynamic configuration change on the region
+ region.onConfigurationChange(newConf);
+
+ // Verify ReadOnly coprocessors are still loaded since the read-only
transition was blocked
+ regionCPHost = region.getCoprocessorHost();
+ assertTrue(
+
CoprocessorConfigurationUtil.areReadOnlyCoprocessorsLoaded(region.getConfiguration(),
+ CoprocessorHost.REGION_COPROCESSOR_CONF_KEY),
+ "ReadOnly coprocessors should remain in the region configuration");
+
+ // Verify new system and user coprocessors were loaded despite the blocked
read-only transition
+
assertNotNull(regionCPHost.findCoprocessor(SimpleRegionObserver.class.getName()),
+ "SimpleRegionObserver should be loaded even when read-only transition is
blocked");
+
assertNotNull(regionCPHost.findCoprocessor(NoOpScanPolicyObserver.class.getName()),
+ "NoOpScanPolicyObserver should be loaded even when read-only transition
is blocked");
+ }
+
+ @Test
+ public void
testRegionServerCannotDisableReadOnlyWhenAnotherClusterIsActive() throws
Exception {
+ // First enable read-only mode
+ setReadOnlyMode(true);
+ assertFalse(activeClusterIdFileExists());
+
+ // Write an active cluster file with a different cluster's data
+ overwriteExistingFile();
+ assertTrue(activeClusterIdFileExists());
+
+ // Attempt to disable read-only mode, but get an exception because another
active cluster
+ // already exists
+
regionServer.getConfiguration().setBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY,
false);
+
+ // RegionServer's updateConfiguration should throw because another cluster
is active
+ assertThrows(ReadOnlyTransitionException.class, () ->
regionServer.updateConfiguration());
+
+ // Verify read-only coprocessors are still loaded on the region server
+ assertTrue(CoprocessorConfigurationUtil.areReadOnlyCoprocessorsLoaded(
+ regionServer.getConfiguration(),
CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY));
+ }
+
+ @Test
+ public void testBlockedThenSuccessfulReadOnlyTransition() throws Exception {
+ // Put cluster in read-only mode with a foreign active cluster file
+ setReadOnlyMode(true);
+ assertFalse(activeClusterIdFileExists());
+ overwriteExistingFile();
+ assertTrue(activeClusterIdFileExists());
+
+ // Attempt to disable read-only mode. This should fail because another
cluster is active.
+ // We need to run updateConfiguration() to ensure the
readOnlyTransitionBlocked boolean in
+ // HBaseServerBase gets set.
+
master.getConfiguration().setBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY,
false);
+ assertThrows(ReadOnlyTransitionException.class, () ->
master.updateConfiguration());
+
regionServer.getConfiguration().setBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY,
false);
+ assertThrows(ReadOnlyTransitionException.class, () ->
regionServer.updateConfiguration());
+
+ // Try to create a table. This should fail because the cluster is still in
read-only mode
+ TableName tableName = TableName.valueOf("testBlockedTransitionTable");
+ TableDescriptor desc = TableDescriptorBuilder.newBuilder(tableName)
+ .setColumnFamily(ColumnFamilyDescriptorBuilder.of("cf")).build();
+ assertThrows(IOException.class, () ->
TEST_UTIL.getAdmin().createTable(desc));
+
+ // Delete the foreign active cluster file
+ fs.delete(activeClusterFile, false);
+ assertFalse(activeClusterIdFileExists());
Review Comment:
nit: so we may need to add a documentation for recovering from blocked
read-only mode, the user need to carefully clean up the incorrect
activeClusterId by using either the filesystem API.
I knew this is strange, is there any hints to choose or tell which one
should be the correct active cluster ID ?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AbstractReadOnlyController.java:
##########
@@ -68,6 +68,33 @@ public void start(CoprocessorEnvironment env) throws
IOException {
public void stop(CoprocessorEnvironment env) {
}
+ /**
+ * Checks whether another cluster is currently active on this storage
location by reading the
+ * {@value HConstants#ACTIVE_CLUSTER_SUFFIX_FILE_NAME} file on the
filesystem.
+ * @param fs the filesystem to read from
+ * @param rootDir the HBase root directory
+ * @param localClusterSuffix the local cluster's ActiveClusterSuffix identity
+ * @return true if the active cluster file exists and belongs to a different
cluster; false if the
+ * file does not exist or belongs to this cluster
+ */
+ public static boolean isAnotherClusterActive(FileSystem fs, Path rootDir,
+ ActiveClusterSuffix localClusterSuffix) {
+ try {
+ ActiveClusterSuffix fileData =
+ FSUtils.getClusterIdFile(fs, rootDir, new
ActiveClusterSuffix.Parser());
+ if (fileData == null) {
+ return false;
+ }
+ return !localClusterSuffix.equals(fileData);
+ } catch (IOException e) {
+ LOG.error(
Review Comment:
nit: I'd suggest to use warning because you're returning normally without
throwing an exception.
```suggestion
LOG.warn(
```
I also found in `manageActiveClusterIdFile` that using error message without
throwing exceptions is the pattern......
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/util/ConfigurationUtil.java:
##########
@@ -116,8 +116,22 @@ public static List<Map.Entry<String, String>>
getKeyValues(Configuration conf, S
return rtn;
}
+ /**
+ * Returns true if the provided Configuration object has
+ * {@link HConstants#HBASE_GLOBAL_READONLY_ENABLED_KEY} set to true; false
otherwise.
+ */
public static boolean isReadOnlyModeEnabledInConf(Configuration conf) {
return conf.getBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY,
HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT);
}
+
+ /**
+ * Returns a copied version of the provided Configuration object that has
+ * {@link HConstants#HBASE_GLOBAL_READONLY_ENABLED_KEY} set to true.
+ */
+ public static Configuration
getReadOnlyEnabledConfigurationCopy(Configuration conf) {
Review Comment:
nit: or maybe use `copyWithReadOnlyModeEnabled` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
