Copilot commented on code in PR #8377:
URL: https://github.com/apache/hbase/pull/8377#discussion_r3581016640
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java:
##########
@@ -610,6 +611,20 @@ public static <T extends ClusterIdFile> T
getClusterIdFile(FileSystem fs, Path r
return cs;
}
+ public static String getClusterIdFromActiveClusterFile(FileSystem fs, Path
rootDir) {
+ String activeClusterId = null;
+ try {
+ ActiveClusterSuffix parsedClusterIdFile =
+ FSUtils.getClusterIdFile(fs, rootDir, new
ActiveClusterSuffix.Parser());
+ if (parsedClusterIdFile != null) {
+ activeClusterId = parsedClusterIdFile.toString();
+ }
Review Comment:
This helper is named like it returns only the cluster ID, but it actually
returns ActiveClusterSuffix.toString() ("<clusterId>:<suffix>"). That makes
log/error messages misleading and may confuse callers; consider returning
ActiveClusterSuffix (or renaming to reflect suffix inclusion) and updating call
sites accordingly.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/HBaseServerBase.java:
##########
@@ -639,11 +650,35 @@ public void updateConfiguration() throws IOException {
LOG.info("Reloading the configuration from disk.");
// Reload the configuration from disk.
preUpdateConfiguration();
+ this.readOnlyTransitionBlocked.set(false);
+ this.blockingActiveClusterId.set(null);
conf.reloadConfiguration();
configurationManager.notifyAllObservers(conf);
+ this.checkForBlockedReadOnlyTransition();
postUpdateConfiguration();
}
+ protected Configuration blockReadOnlyTransition(Configuration updatedConf,
+ String activeClusterId) {
+ this.blockingActiveClusterId.set(activeClusterId);
+ LOG.error(
+ "Cannot disable read-only mode. The {} file contains a different cluster
ID ({}), which means "
+ + "that cluster is already the active cluster. Reverting {} to true",
+ HConstants.ACTIVE_CLUSTER_SUFFIX_FILE_NAME,
this.blockingActiveClusterId.get(),
+ HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY);
Review Comment:
The LOG.error format string here exceeds the repo's 100-character Checkstyle
LineLength limit, which can cause the build to fail. Please wrap the message
across multiple concatenated string literals so each source line stays under
100 characters.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java:
##########
@@ -4520,11 +4521,33 @@ public void onConfigurationChange(Configuration
updatedConf) {
boolean originalIsReadOnlyEnabled = CoprocessorConfigurationUtil
.areReadOnlyCoprocessorsLoaded(this.conf,
CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY);
+ boolean newReadOnlyEnabled =
ConfigurationUtil.isReadOnlyModeEnabledInConf(updatedConf);
- // 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,
+ // The updatedConf is potentially a shared Configuration object, so we do
not want to directly
+ // revert its read-only value if another active cluster already exists.
For now, we reference
+ // updatedConf and create a copy for modification below if necessary.
+ Configuration confForCoprocessors = updatedConf;
Review Comment:
This comment suggests we avoid reverting a shared Configuration, but in the
blocked-transition path we end up syncing the defensive copy back into
this.conf (which is often the same object as updatedConf in production). Please
adjust the comment to reflect the actual behavior so future maintainers don't
assume updatedConf remains untouched.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java:
##########
@@ -3505,11 +3508,34 @@ public void onConfigurationChange(Configuration
updatedConf) {
boolean originalIsReadOnlyEnabled = CoprocessorConfigurationUtil
.areReadOnlyCoprocessorsLoaded(this.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,
+ boolean newReadOnlyEnabled =
ConfigurationUtil.isReadOnlyModeEnabledInConf(updatedConf);
+
+ // The updatedConf is potentially a shared Configuration object, so we do
not want to directly
+ // revert its read-only value if another active cluster already exists.
For now, we reference
+ // updatedConf and create a copy for modification below if necessary.
+ Configuration confForCoprocessors = updatedConf;
Review Comment:
This comment suggests we avoid reverting a shared Configuration, but in the
blocked-transition path we end up syncing the defensive copy back into
this.conf (which is often the same object as updatedConf in production). Please
adjust the comment to reflect the actual behavior so future maintainers don't
assume updatedConf remains untouched.
--
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]
