anmolnar commented on code in PR #7437:
URL: https://github.com/apache/hbase/pull/7437#discussion_r2515224768
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/ReadOnlyController.java:
##########
@@ -400,14 +415,63 @@ 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);
+
+ if (fs.exists(activeClusterFile)) {
+ if (fs.delete(activeClusterFile, false)) {
+ LOG.info("Successfully deleted active cluster file: {}",
activeClusterFile);
+ } else {
+ LOG.error(
+ "Failed to delete active cluster file: {}. "
+ + "Read-only flag will be updated, but file system state is
inconsistent.",
+ activeClusterFile);
+ }
+ } else {
+ LOG.debug("Active cluster file not present, nothing to delete.");
Review Comment:
This is an error situation, right? Because you're transitioning from R/W ->
R/O mode and trying to delete the file which should be there. I would just
delete it right away an log the error like this:
```java
try {
fs.delete(activeClusterFile, false));
LOG.info("Successfully deleted active cluster file: {}",
activeClusterFile);
} catch (IOException e) {
LOG.error(...);
}
```
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/ReadOnlyController.java:
##########
@@ -400,14 +415,63 @@ 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);
+
+ if (fs.exists(activeClusterFile)) {
+ if (fs.delete(activeClusterFile, false)) {
+ LOG.info("Successfully deleted active cluster file: {}",
activeClusterFile);
+ } else {
+ LOG.error(
+ "Failed to delete active cluster file: {}. "
+ + "Read-only flag will be updated, but file system state is
inconsistent.",
+ activeClusterFile);
+ }
+ } else {
+ LOG.debug("Active cluster file not present, nothing to delete.");
+ }
+ } else {
+ // DISABLING READ-ONLY (true -> false), create the active cluster file
id file
+ try (FSDataOutputStream stream = fs.create(activeClusterFile, true)) {
+ stream.writeUTF(new String(mfs.getSuffixFileDataToWrite(),
StandardCharsets.UTF_8));
+ LOG.debug(
+ "Global read-only mode is being DISABLED. Successfully created
active "
+ + "cluster file {} with suffix {}",
+ activeClusterFile, mfs.getSuffixFileDataToWrite());
+ }
+ }
+ } 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,
+ boolean newValue =
conf.getBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY,
HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT);
- if (this.globalReadOnlyEnabled != maybeUpdatedConfValue) {
- this.globalReadOnlyEnabled = maybeUpdatedConfValue;
- LOG.info("Config {} has been dynamically changed to {}",
+ if (this.globalReadOnlyEnabled != newValue) {
+ if (this.masterServices != null) {
+ manageActiveClusterIdFile(newValue);
+ } else {
+ LOG.debug("MasterServices is not initialized. Cannot perform file
operations for "
Review Comment:
This indicates a problematic situation to me, but it's actually not. I don't
think you log anything here, but if you really want to, rephrase to something
like 'Global R/O flag changed, but not running on master'
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/ReadOnlyController.java:
##########
@@ -400,14 +415,63 @@ 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);
+
+ if (fs.exists(activeClusterFile)) {
+ if (fs.delete(activeClusterFile, false)) {
+ LOG.info("Successfully deleted active cluster file: {}",
activeClusterFile);
+ } else {
+ LOG.error(
+ "Failed to delete active cluster file: {}. "
+ + "Read-only flag will be updated, but file system state is
inconsistent.",
+ activeClusterFile);
+ }
+ } else {
+ LOG.debug("Active cluster file not present, nothing to delete.");
+ }
+ } else {
+ // DISABLING READ-ONLY (true -> false), create the active cluster file
id file
+ try (FSDataOutputStream stream = fs.create(activeClusterFile, true)) {
+ stream.writeUTF(new String(mfs.getSuffixFileDataToWrite(),
StandardCharsets.UTF_8));
+ LOG.debug(
+ "Global read-only mode is being DISABLED. Successfully created
active "
+ + "cluster file {} with suffix {}",
+ activeClusterFile, mfs.getSuffixFileDataToWrite());
+ }
+ }
+ } 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,
+ boolean newValue =
conf.getBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY,
Review Comment:
nit: I think the previous name of the variable is fine, no need to rename.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/ReadOnlyController.java:
##########
@@ -400,14 +415,63 @@ 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);
+
+ if (fs.exists(activeClusterFile)) {
+ if (fs.delete(activeClusterFile, false)) {
+ LOG.info("Successfully deleted active cluster file: {}",
activeClusterFile);
+ } else {
+ LOG.error(
+ "Failed to delete active cluster file: {}. "
+ + "Read-only flag will be updated, but file system state is
inconsistent.",
+ activeClusterFile);
+ }
+ } else {
+ LOG.debug("Active cluster file not present, nothing to delete.");
+ }
+ } else {
+ // DISABLING READ-ONLY (true -> false), create the active cluster file
id file
+ try (FSDataOutputStream stream = fs.create(activeClusterFile, true)) {
+ stream.writeUTF(new String(mfs.getSuffixFileDataToWrite(),
StandardCharsets.UTF_8));
Review Comment:
You already have a helper method for this
`FSUtils.setActiveClusterSuffix()`. Why not using it?
--
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]
