[GitHub] [hbase] ndimiduk commented on a diff in pull request #5036: HBASE-27652 Client-side lock contention around Configuration when using read replica regions

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

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


##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/RpcRetryingCallerFactory.java:
##########
@@ -71,30 +71,41 @@ public <T> RpcRetryingCaller<T> newCaller() {
       interceptor, startLogErrorsCnt, connectionConf.getRpcTimeout(), metrics);
   }
 
-  public static RpcRetryingCallerFactory instantiate(Configuration configuration,
+  @RestrictedApi(explanation = "Should only be called on process initialization", link = "",

Review Comment:
   Avoid calling this method as much as is reasonable.
   
   The `HRegionServer` call site is particularly insidious. It is called from the `HRegionServer` constructor and looks like this,
   
   ```java
         rpcRetryingCallerFactory = RpcRetryingCallerFactory.instantiate(this.conf,
           clusterConnection == null ? null : clusterConnection.getConnectionMetrics());
   ```
   
   I think that `clusterConnection` is always null here, because `clusterConnection` is instantiated in `protected synchronized void setupClusterConnection()`, which, as far as I can tell, is only called from `public void run()`.



##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/RpcRetryingCallerFactory.java:
##########
@@ -17,32 +17,32 @@
  */
 package org.apache.hadoop.hbase.client;
 
+import com.google.errorprone.annotations.RestrictedApi;
 import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.HBaseInterfaceAudience;
 import org.apache.hadoop.hbase.util.ReflectionUtils;
 import org.apache.yetus.audience.InterfaceAudience;
 
 /**
  * Factory to create an {@link RpcRetryingCaller}
  */
-@InterfaceAudience.Private
+@InterfaceAudience.LimitedPrivate(HBaseInterfaceAudience.CONFIG)

Review Comment:
   Upgrading the InterfaceAudience here because in fact this class is exposed via configuration and can be overridden by the site installation. Thus we are bound to an existing constructor interface.



-- 
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