[GitHub] [hbase] virajjasani commented on a diff in pull request #4874: HBASE-27466: Making metrics instance containing one or more connections.

[GitBox <gi...@apache.org>]

virajjasani commented on code in PR #4874:
URL: https://github.com/apache/hbase/pull/4874#discussion_r1036578735


##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetricsConnection.java:
##########
@@ -457,6 +513,27 @@ public void incrementServerOverloadedBackoffTime(long time, TimeUnit timeUnit) {
     overloadedBackoffTimer.update(time, timeUnit);
   }
 
+  /** Return the connection count of the metrics within a scope */
+  public long getConnectionCount() {
+    return connectionCount.getCount();
+  }
+
+  /** Increment the connection count of the metrics within a scope */
+  private void incrConnectionCount() {
+    connectionCount.inc();
+  }
+
+  /** Decrement the connection count of the metrics within a scope */
+  private void decrConnectionCount() {
+    connectionCount.dec();
+  }
+
+  /** Add thread pools of additional connections to the metrics */
+  private void addThreadPools(Supplier batchPool, Supplier metaPool) {

Review Comment:
   Good to replace raw use with `Supplier<ThreadPoolExecutor> batchPool, Supplier<ThreadPoolExecutor> metaPool`



##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetricsConnection.java:
##########
@@ -295,8 +328,13 @@ public Counter newMetric(Class<?> clazz, String name, String scope) {
     }
   };
 
+  // List of thread pool per connection of the metrics.
+  private List<Supplier<ThreadPoolExecutor>> batchPools = new ArrayList<>();
+  private List<Supplier<ThreadPoolExecutor>> metaPools = new ArrayList<>();

Review Comment:
   nit: `final` for both?



##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetricsConnection.java:
##########
@@ -331,30 +369,52 @@ public Counter newMetric(Class<?> clazz, String name, String scope) {
   protected final ConcurrentMap<String, Counter> rpcCounters =
     new ConcurrentHashMap<>(CAPACITY, LOAD_FACTOR, CONCURRENCY_LEVEL);
 
-  MetricsConnection(String scope, Supplier<ThreadPoolExecutor> batchPool,
+  private MetricsConnection(String scope, Supplier<ThreadPoolExecutor> batchPool,
     Supplier<ThreadPoolExecutor> metaPool) {
     this.scope = scope;
+    addThreadPools(batchPool, metaPool);
     this.registry = new MetricRegistry();
     this.registry.register(getExecutorPoolName(), new RatioGauge() {
       @Override
       protected Ratio getRatio() {
-        ThreadPoolExecutor pool = batchPool.get();
-        if (pool == null) {
-          return Ratio.of(0, 0);
+        int numerator = 0;
+        int denominator = 0;
+        for (Supplier<ThreadPoolExecutor> poolSupplier : batchPools) {
+          ThreadPoolExecutor pool = poolSupplier.get();
+          if (pool != null) {
+            int activeCount = pool.getActiveCount();
+            int maxPoolSize = pool.getMaximumPoolSize();
+            /* The max thread usage ratio among batch pools of all connections */
+            if (numerator == 0 || (numerator * maxPoolSize) < (activeCount * denominator)) {
+              numerator = activeCount;
+              denominator = maxPoolSize;
+            }

Review Comment:
   Just for better understanding, is there any doc or example for this new computation?



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