anmolnar commented on code in PR #7437:
URL: https://github.com/apache/hbase/pull/7437#discussion_r2543011861
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/ReadOnlyController.java:
##########
@@ -411,14 +424,53 @@ public void
preCleanupBulkLoad(ObserverContext<RegionCoprocessorEnvironment> ctx
BulkLoadObserver.super.preCleanupBulkLoad(ctx);
}
+ private void manageActiveClusterIdFile(boolean newValue) {
+ MasterFileSystem mfs = this.masterServices.getMasterFileSystem();
+ FileSystem fs = mfs.getFileSystem();
+ Path rootDir = mfs.getRootDir();
+ Path activeClusterFile = new Path(rootDir,
HConstants.ACTIVE_CLUSTER_SUFFIX_FILE_NAME);
+
+ try {
+ if (newValue) {
+ // ENABLING READ-ONLY (false -> true), delete the active cluster file.
+ LOG.debug("Global read-only mode is being ENABLED. Deleting active
cluster file: {}",
+ activeClusterFile);
+ try {
+ fs.delete(activeClusterFile, false);
+ LOG.info("Successfully deleted active cluster file: {}",
activeClusterFile);
+ } catch (IOException e) {
+ LOG.error(
+ "Failed to delete active cluster file: {}. "
+ + "Read-only flag will be updated, but file system state is
inconsistent.",
+ activeClusterFile);
+ }
+ } else {
+ // DISABLING READ-ONLY (true -> false), create the active cluster file
id file
+ int wait =
mfs.getConfiguration().getInt(HConstants.THREAD_WAKE_FREQUENCY, 10 * 1000);
+ FSUtils.setActiveClusterSuffix(fs, rootDir,
mfs.getSuffixFileDataToWrite(), wait);
+ }
+ } catch (IOException e) {
+ // We still update the flag, but log that the operation failed.
+ LOG.error("Failed to perform file operation for read-only switch. "
+ + "Flag will be updated, but file system state may be inconsistent.",
e);
+ this.globalReadOnlyEnabled = newValue;
+ LOG.info("Config {} has been dynamically changed to {}. Encountered FS
error",
+ HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY,
this.globalReadOnlyEnabled);
+ }
+ }
+
/* ---- ConfigurationObserver Overrides ---- */
- @Override
public void onConfigurationChange(Configuration conf) {
boolean maybeUpdatedConfValue =
conf.getBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY,
HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT);
if (this.globalReadOnlyEnabled != maybeUpdatedConfValue) {
+ if (this.masterServices != null) {
+ manageActiveClusterIdFile(maybeUpdatedConfValue);
+ } else {
+ LOG.debug("Global R/O flag changed, but not running on master");
+ }
this.globalReadOnlyEnabled = maybeUpdatedConfValue;
- LOG.info("Config {} has been dynamically changed to {}",
+ LOG.debug("Config {} has been dynamically changed to {}. (No FS ops
performed 1)",
Review Comment:
> (No FS ops performed 1)
What does it mean and why have you changed the message's severity to debug
level?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/ReadOnlyController.java:
##########
@@ -84,7 +91,13 @@ private void internalReadOnlyGuard() throws IOException {
@Override
public void start(CoprocessorEnvironment env) throws IOException {
-
+ if (env instanceof MasterCoprocessorEnvironment) {
+ this.masterServices = ((MasterCoprocessorEnvironment)
env).getMasterServices();
+ LOG.info("ReadOnlyController obtained MasterServices reference from
start().");
+ } else {
+ LOG.warn("ReadOnlyController loaded in a non-Master environment. "
Review Comment:
This should not be a warning level message. It's completely normal that
ReadOnlyController runs on both Master and Region Servers. There's nothing to
warn the user about. It could be just a debug level message.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestReadOnlyController.java:
##########
@@ -154,6 +159,22 @@ private static void notifyObservers() {
hRegionServer.getConfigurationManager().notifyAllObservers(conf);
}
+ private static boolean isActiveClusterIdFileExists() {
+ MasterFileSystem mfs = hMaster.getMasterFileSystem();
+ Path rootDir = mfs.getRootDir();
+ FileSystem fs = mfs.getFileSystem();
+ Path activeClusterFile = new Path(rootDir,
HConstants.ACTIVE_CLUSTER_SUFFIX_FILE_NAME);
+ boolean status = false;
+ try {
+ if (fs.exists(activeClusterFile)) {
Review Comment:
Don't catch the exception, because you will hide a potential error situation
with the test.
```java
return fs.exists(activeClusterFile);
```
should be fine, because the `FileNotFoundException` is already handled in
the method. Other IOExceptions should be propagated back to the test runner.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/ReadOnlyController.java:
##########
@@ -411,14 +424,53 @@ public void
preCleanupBulkLoad(ObserverContext<RegionCoprocessorEnvironment> ctx
BulkLoadObserver.super.preCleanupBulkLoad(ctx);
}
+ private void manageActiveClusterIdFile(boolean newValue) {
+ MasterFileSystem mfs = this.masterServices.getMasterFileSystem();
+ FileSystem fs = mfs.getFileSystem();
+ Path rootDir = mfs.getRootDir();
+ Path activeClusterFile = new Path(rootDir,
HConstants.ACTIVE_CLUSTER_SUFFIX_FILE_NAME);
+
+ try {
+ if (newValue) {
+ // ENABLING READ-ONLY (false -> true), delete the active cluster file.
+ LOG.debug("Global read-only mode is being ENABLED. Deleting active
cluster file: {}",
+ activeClusterFile);
+ try {
+ fs.delete(activeClusterFile, false);
+ LOG.info("Successfully deleted active cluster file: {}",
activeClusterFile);
+ } catch (IOException e) {
+ LOG.error(
+ "Failed to delete active cluster file: {}. "
+ + "Read-only flag will be updated, but file system state is
inconsistent.",
+ activeClusterFile);
+ }
+ } else {
+ // DISABLING READ-ONLY (true -> false), create the active cluster file
id file
+ int wait =
mfs.getConfiguration().getInt(HConstants.THREAD_WAKE_FREQUENCY, 10 * 1000);
+ FSUtils.setActiveClusterSuffix(fs, rootDir,
mfs.getSuffixFileDataToWrite(), wait);
+ }
+ } catch (IOException e) {
+ // We still update the flag, but log that the operation failed.
+ LOG.error("Failed to perform file operation for read-only switch. "
+ + "Flag will be updated, but file system state may be inconsistent.",
e);
+ this.globalReadOnlyEnabled = newValue;
Review Comment:
It's already done in the event handler in line 472.
Please remove it from here.
--
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]
