Re: [PR] HBASE-26268: Provide coprocessor hooks for updateConfiguration and clearRegionBlockCache [hbase]

["ndimiduk (via GitHub)" <gi...@apache.org>]

ndimiduk commented on code in PR #5593:
URL: https://github.com/apache/hbase/pull/5593#discussion_r1441510222


##########
hbase-server/src/main/java/org/apache/hadoop/hbase/HBaseServerBase.java:
##########
@@ -614,11 +616,41 @@ public ConfigurationManager getConfigurationManager() {
   /**
    * Reload the configuration from disk.
    */
-  public void updateConfiguration() {
+  public void updateConfiguration() throws IOException {

Review Comment:
   The signal handler should be modified to also go through this method. It is registered in the inaccurately named `setupWindows` method. But in this case, there's no user identity to present for permission enforcement. You may have to explicitly set in place a system user for this call...



##########
hbase-server/src/main/java/org/apache/hadoop/hbase/HBaseServerBase.java:
##########
@@ -614,11 +616,41 @@ public ConfigurationManager getConfigurationManager() {
   /**
    * Reload the configuration from disk.
    */
-  public void updateConfiguration() {
+  public void updateConfiguration() throws IOException {
     LOG.info("Reloading the configuration from disk.");
     // Reload the configuration from disk.
+    preUpdateConfiguration();
     conf.reloadConfiguration();
     configurationManager.notifyAllObservers(conf);
+    postUpdateConfiguration();
+  }
+
+  private void preUpdateConfiguration() throws IOException {
+    if (rpcServices instanceof RSRpcServices && rpcServices.server != null) {
+      RSRpcServices rsRpcServices = (RSRpcServices) rpcServices;
+      if (rsRpcServices.server.getRegionServerCoprocessorHost() != null) {
+        rsRpcServices.server.getRegionServerCoprocessorHost().preUpdateConfiguration(conf);

Review Comment:
   Reaching for the `cpHost` via the `rpcServices` instance is a little strange. Should this class instead have an `abstract protected` accessor method that can be used to retrieve the instance?  I guess unless we replicate this class hierarchy in the CoprocessorHost, we end up doing casts anyway.
   
   Meh.



##########
hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/MasterObserver.java:
##########
@@ -1873,4 +1874,12 @@ default void preHasUserPermissions(ObserverContext<MasterCoprocessorEnvironment>
   default void postHasUserPermissions(ObserverContext<MasterCoprocessorEnvironment> ctx,
     String userName, List<Permission> permissions) throws IOException {
   }
+
+  default void preUpdateMasterConfiguration(ObserverContext<MasterCoprocessorEnvironment> ctx,

Review Comment:
   These methods are exposed to our public API, so a javadoc string would be appreciated.



##########
hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionServerObserver.java:
##########
@@ -169,4 +171,22 @@ default void postReplicationSinkBatchMutate(
 
   }
 
+  default void preClearRegionBlockCache(ObserverContext<RegionServerCoprocessorEnvironment> ctx)

Review Comment:
   Likewise re: javadocs.



-- 
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: issues-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org