Address comments in HBASE-29081

hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java

@@ -1666,9 +1666,7 @@ public enum OperationStatusCode {
   public final static boolean REJECT_DECOMMISSIONED_HOSTS_DEFAULT = false;
 
   /**
-   * Adds a suffix to the meta table name: value=’test’ -> ‘hbase:meta_test’ Added in HBASE-XXXXX to
-   * support having multiple hbase:meta tables (with distinct names )to enable storage sharing by
-   * more than one clusters.
+   * Adds a suffix to the meta table name: value='test' -> ‘hbase:meta_test’
    */
   public final static String HBASE_META_TABLE_SUFFIX = "hbase.meta.table.suffix";
 

hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java

@@ -36,8 +36,6 @@
 import org.apache.hadoop.hbase.CoprocessorEnvironment;
 import org.apache.hadoop.hbase.DoNotRetryIOException;
 import org.apache.hadoop.hbase.HConstants;
-import org.apache.hadoop.hbase.conf.ConfigurationManager;
-import org.apache.hadoop.hbase.conf.ConfigurationObserver;
 import org.apache.hadoop.hbase.ipc.RpcServer;
 import org.apache.hadoop.hbase.security.User;
 import org.apache.hadoop.hbase.util.CoprocessorClassLoader;
@@ -132,39 +130,6 @@ public Set<String> getCoprocessorClassNames() {
     return returnValue;
   }
 
-  /**
-   * Used to help make the relevant loaded coprocessors dynamically configurable by registering them
-   * to the {@link ConfigurationManager}. Coprocessors are considered "relevant" if they implement
-   * the {@link ConfigurationObserver} interface.
-   * @param configurationManager the ConfigurationManager the coprocessors get registered to
-   */
-  public void registerConfigurationObservers(ConfigurationManager configurationManager) {
-    Coprocessor foundCp;
-    Set<String> coprocessors = this.getCoprocessors();
-    for (String cp : coprocessors) {
-      foundCp = this.findCoprocessor(cp);
-      if (foundCp instanceof ConfigurationObserver) {
-        configurationManager.registerObserver((ConfigurationObserver) foundCp);
-      }
-    }
-  }
-
-  /**
-   * Deregisters relevant coprocessors from the {@link ConfigurationManager}. Coprocessors are
-   * considered "relevant" if they implement the {@link ConfigurationObserver} interface.
-   * @param configurationManager the ConfigurationManager the coprocessors get deregistered from
-   */
-  public void deregisterConfigurationObservers(ConfigurationManager configurationManager) {
-    Coprocessor foundCp;
-    Set<String> coprocessors = this.getCoprocessors();
-    for (String cp : coprocessors) {
-      foundCp = this.findCoprocessor(cp);
-      if (foundCp instanceof ConfigurationObserver) {
-        configurationManager.deregisterObserver((ConfigurationObserver) foundCp);
-      }
-    }
-  }
-
   /**
    * Load system coprocessors once only. Read the class names from configuration. Called by
    * constructor.

hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java

@@ -18,8 +18,6 @@
 package org.apache.hadoop.hbase.master;
 
 import static org.apache.hadoop.hbase.HConstants.DEFAULT_HBASE_SPLIT_COORDINATED_BY_ZK;
-import static org.apache.hadoop.hbase.HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT;
-import static org.apache.hadoop.hbase.HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY;
 import static org.apache.hadoop.hbase.HConstants.HBASE_MASTER_LOGCLEANER_PLUGINS;
 import static org.apache.hadoop.hbase.HConstants.HBASE_SPLIT_WAL_COORDINATED_BY_ZK;
 import static org.apache.hadoop.hbase.master.cleaner.HFileCleaner.CUSTOM_POOL_SIZE;
@@ -500,8 +498,6 @@ public class HMaster extends HBaseServerBase<MasterRpcServices> implements Maste
   public static final String WARMUP_BEFORE_MOVE = "hbase.master.warmup.before.move";
   private static final boolean DEFAULT_WARMUP_BEFORE_MOVE = true;
 
-  private volatile boolean isGlobalReadOnlyEnabled;
-
   /**
    * Use RSProcedureDispatcher instance to initiate master -> rs remote procedure execution. Use
    * this config to extend RSProcedureDispatcher (mainly for testing purpose).
@@ -587,8 +583,6 @@ public HMaster(final Configuration conf) throws IOException {
           getChoreService().scheduleChore(clusterStatusPublisherChore);
         }
       }
-      this.isGlobalReadOnlyEnabled =
-        conf.getBoolean(HBASE_GLOBAL_READONLY_ENABLED_KEY, HBASE_GLOBAL_READONLY_ENABLED_DEFAULT);
       this.activeMasterManager = createActiveMasterManager(zooKeeper, serverName, this);
       cachedClusterId = new CachedClusterId(this, conf);
       this.regionServerTracker = new RegionServerTracker(zooKeeper, this);
@@ -622,12 +616,6 @@ protected String getUseThisHostnameInstead(Configuration conf) {
   private void registerConfigurationObservers() {
     configurationManager.registerObserver(this.rpcServices);
     configurationManager.registerObserver(this);
-    if (cpHost != null) {
-      cpHost.registerConfigurationObservers(configurationManager);
-    } else {
-      LOG.warn("Could not register HMaster coprocessors to the ConfigurationManager because "
-        + "MasterCoprocessorHost is null");
-    }
   }
 
   // Main run loop. Calls through to the regionserver run loop AFTER becoming active Master; will
@@ -636,6 +624,7 @@ private void registerConfigurationObservers() {
   public void run() {
     try {
       installShutdownHook();
+      registerConfigurationObservers();
       Threads.setDaemonThreadRunning(new Thread(TraceUtil.tracedRunnable(() -> {
         try {
           int infoPort = putUpJettyServer();
@@ -1099,7 +1088,7 @@ private void finishActiveMasterInitialization() throws IOException, InterruptedE
       CoprocessorConfigurationUtil.syncReadOnlyConfigurations(conf,
         CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY);
       AbstractReadOnlyController.manageActiveClusterIdFile(
-        ConfigurationUtil.isReadOnlyModeEnabled(conf), this.getMasterFileSystem());
+        ConfigurationUtil.isReadOnlyModeEnabledInConf(conf), this.getMasterFileSystem());
       initializeCoprocessorHost(conf);
     } else {
       // start an in process region server for carrying system regions
@@ -4506,15 +4495,22 @@ public void onConfigurationChange(Configuration newConf) {
     // append the quotas observer back to the master coprocessor key
     setQuotasObserver(newConf);
 
-    boolean originalIsReadOnlyEnabled = this.isGlobalReadOnlyEnabled;
+    boolean originalIsReadOnlyEnabled = CoprocessorConfigurationUtil
+      .areReadOnlyCoprocessorsLoaded(this.conf, CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY);
 
-    CoprocessorConfigurationUtil.maybeUpdateCoprocessors(newConf, this.isGlobalReadOnlyEnabled,
+    CoprocessorConfigurationUtil.maybeUpdateCoprocessors(newConf, originalIsReadOnlyEnabled,
       this.cpHost, CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY, this.maintenanceMode,
-      this.toString(), val -> this.isGlobalReadOnlyEnabled = val,
-      conf -> initializeCoprocessorHost(newConf));
+      this.toString(), conf -> {
+        this.initializeCoprocessorHost(conf);
+        CoprocessorConfigurationUtil.updateCoprocessorListInConf(this.conf, conf,
+          CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY);
+      });
+
+    boolean maybeUpdatedReadOnlyMode = CoprocessorConfigurationUtil
+      .areReadOnlyCoprocessorsLoaded(this.conf, CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY);
 
-    if (this.isGlobalReadOnlyEnabled != originalIsReadOnlyEnabled) {
-      AbstractReadOnlyController.manageActiveClusterIdFile(this.isGlobalReadOnlyEnabled,
+    if (maybeUpdatedReadOnlyMode != originalIsReadOnlyEnabled) {
+      AbstractReadOnlyController.manageActiveClusterIdFile(maybeUpdatedReadOnlyMode,
         this.getMasterFileSystem());
     }
   }
@@ -4617,7 +4613,6 @@ private void setQuotasObserver(Configuration conf) {
   private void initializeCoprocessorHost(Configuration conf) {
     // initialize master side coprocessors before we start handling requests
     this.cpHost = new MasterCoprocessorHost(this, conf);
-    registerConfigurationObservers();
   }
 
   @Override

hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java

@@ -396,7 +396,7 @@ public void logFileSystemState(Logger log) throws IOException {
 
   private void negotiateActiveClusterSuffixFile(long wait) throws IOException {
     this.activeClusterSuffix = ActiveClusterSuffix.fromConfig(conf, getClusterId());
-    if (!ConfigurationUtil.isReadOnlyModeEnabled(conf)) {
+    if (!ConfigurationUtil.isReadOnlyModeEnabledInConf(conf)) {
       try {
         // verify the contents against the config set
         ActiveClusterSuffix acs =

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactSplit.java

@@ -346,7 +346,7 @@ protected void requestCompactionInternal(HRegion region, HStore store, String wh
     }
 
     // Should not allow compaction if cluster is in read-only mode
-    if (ConfigurationUtil.isReadOnlyModeEnabled(conf)) {
+    if (ConfigurationUtil.isReadOnlyModeEnabledInConf(conf)) {
       LOG.info("Ignoring compaction request for " + region + ",because read-only mode is on.");
       return;
     }
@@ -451,7 +451,7 @@ private Optional<CompactionContext> selectCompaction(HRegion region, HStore stor
     }
 
     // Should not allow compaction if cluster is in read-only mode
-    if (ConfigurationUtil.isReadOnlyModeEnabled(conf)) {
+    if (ConfigurationUtil.isReadOnlyModeEnabledInConf(conf)) {
       LOG.info(String.format("Compaction request skipped as read-only mode is on"));
       return Optional.empty();
     }

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java

@@ -17,8 +17,6 @@
  */
 package org.apache.hadoop.hbase.regionserver;
 
-import static org.apache.hadoop.hbase.HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT;
-import static org.apache.hadoop.hbase.HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY;
 import static org.apache.hadoop.hbase.HConstants.REPLICATION_SCOPE_LOCAL;
 import static org.apache.hadoop.hbase.regionserver.HStoreFile.MAJOR_COMPACTION_KEY;
 import static org.apache.hadoop.hbase.trace.HBaseSemanticAttributes.REGION_NAMES_KEY;
@@ -392,8 +390,6 @@ public class HRegion implements HeapSize, PropagatingConfigurationObserver, Regi
   private Path regionWalDir;
   private FileSystem walFS;
 
-  private volatile boolean isGlobalReadOnlyEnabled;
-
   // set to true if the region is restored from snapshot for reading by ClientSideRegionScanner
   private boolean isRestoredRegion = false;
 
@@ -944,8 +940,6 @@ public HRegion(final HRegionFileSystem fs, final WAL wal, final Configuration co
 
     decorateRegionConfiguration(conf);
 
-    this.isGlobalReadOnlyEnabled =
-      conf.getBoolean(HBASE_GLOBAL_READONLY_ENABLED_KEY, HBASE_GLOBAL_READONLY_ENABLED_DEFAULT);
     CoprocessorConfigurationUtil.syncReadOnlyConfigurations(this.conf,
       CoprocessorHost.REGION_COPROCESSOR_CONF_KEY);
 
@@ -8993,11 +8987,16 @@ IOException throwOnInterrupt(Throwable t) {
   public void onConfigurationChange(Configuration newConf) {
     this.storeHotnessProtector.update(newConf);
 
-    CoprocessorConfigurationUtil.maybeUpdateCoprocessors(newConf, this.isGlobalReadOnlyEnabled,
+    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(),
-      val -> this.isGlobalReadOnlyEnabled = val, conf -> {
+      conf -> {
         decorateRegionConfiguration(conf);
-        this.coprocessorHost = new RegionCoprocessorHost(this, rsServices, newConf);
+        this.coprocessorHost = new RegionCoprocessorHost(this, rsServices, conf);
+        CoprocessorConfigurationUtil.updateCoprocessorListInConf(this.conf, conf,
+          CoprocessorHost.REGION_COPROCESSOR_CONF_KEY);
       });
   }
 
@@ -9008,12 +9007,6 @@ public void onConfigurationChange(Configuration newConf) {
   public void registerChildren(ConfigurationManager manager) {
     configurationManager = manager;
     stores.values().forEach(manager::registerObserver);
-    if (coprocessorHost != null) {
-      coprocessorHost.registerConfigurationObservers(manager);
-    } else {
-      LOG.warn("Could not register HRegion coprocessors to the ConfigurationManager because "
-        + "RegionCoprocessorHost is null");
-    }
   }
 
   /**
@@ -9022,12 +9015,6 @@ public void registerChildren(ConfigurationManager manager) {
   @Override
   public void deregisterChildren(ConfigurationManager manager) {
     stores.values().forEach(configurationManager::deregisterObserver);
-    if (coprocessorHost != null) {
-      coprocessorHost.deregisterConfigurationObservers(manager);
-    } else {
-      LOG.warn("Could not deregister HRegion coprocessors from the ConfigurationManager because "
-        + "RegionCoprocessorHost is null");
-    }
   }
 
   @Override

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java

@@ -20,8 +20,6 @@
 import static org.apache.hadoop.hbase.HConstants.DEFAULT_HBASE_SPLIT_COORDINATED_BY_ZK;
 import static org.apache.hadoop.hbase.HConstants.DEFAULT_HBASE_SPLIT_WAL_MAX_SPLITTER;
 import static org.apache.hadoop.hbase.HConstants.DEFAULT_SLOW_LOG_SYS_TABLE_CHORE_DURATION;
-import static org.apache.hadoop.hbase.HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT;
-import static org.apache.hadoop.hbase.HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY;
 import static org.apache.hadoop.hbase.HConstants.HBASE_SPLIT_WAL_COORDINATED_BY_ZK;
 import static org.apache.hadoop.hbase.HConstants.HBASE_SPLIT_WAL_MAX_SPLITTER;
 import static org.apache.hadoop.hbase.master.waleventtracker.WALEventTrackerTableCreator.WAL_EVENT_TRACKER_ENABLED_DEFAULT;
@@ -548,9 +546,6 @@ public HRegionServer(final Configuration conf) throws IOException {
       uncaughtExceptionHandler =
         (t, e) -> abort("Uncaught exception in executorService thread " + t.getName(), e);
 
-      this.isGlobalReadOnlyEnabled =
-        conf.getBoolean(HBASE_GLOBAL_READONLY_ENABLED_KEY, HBASE_GLOBAL_READONLY_ENABLED_DEFAULT);
-
       // If no master in cluster, skip trying to track one or look for a cluster status.
       if (!this.masterless) {
         masterAddressTracker = new MasterAddressTracker(getZooKeeper(), this);
@@ -2163,12 +2158,6 @@ private void registerConfigurationObservers() {
     configurationManager.registerObserver(this.rpcServices);
     configurationManager.registerObserver(this.prefetchExecutorNotifier);
     configurationManager.registerObserver(this);
-    if (rsHost != null) {
-      rsHost.registerConfigurationObservers(configurationManager);
-    } else {
-      LOG.warn("Could not register HRegionServer coprocessors to the ConfigurationManager because "
-        + "RegionServerCoprocessorHost is null");
-    }
   }
 
   /*
@@ -3493,10 +3482,16 @@ public void onConfigurationChange(Configuration newConf) {
       LOG.warn("Failed to initialize SuperUsers on reloading of the configuration");
     }
 
-    CoprocessorConfigurationUtil.maybeUpdateCoprocessors(newConf, this.isGlobalReadOnlyEnabled,
+    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(),
-      val -> this.isGlobalReadOnlyEnabled = val,
-      conf -> this.rsHost = new RegionServerCoprocessorHost(this, newConf));
+      conf -> {
+        this.rsHost = new RegionServerCoprocessorHost(this, conf);
+        CoprocessorConfigurationUtil.updateCoprocessorListInConf(this.conf, conf,
+          CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY);
+      });
   }
 
   @Override

hbase-server/src/main/java/org/apache/hadoop/hbase/util/ConfigurationUtil.java

@@ -116,7 +116,7 @@ public static List<Map.Entry<String, String>> getKeyValues(Configuration conf, S
     return rtn;
   }
 
-  public static boolean isReadOnlyModeEnabled(Configuration conf) {
+  public static boolean isReadOnlyModeEnabledInConf(Configuration conf) {
     return conf.getBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY,
       HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT);
   }