d-c-manning commented on code in PR #4874:
URL: https://github.com/apache/hbase/pull/4874#discussion_r1020611143
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetricsConnection.java:
##########
@@ -334,27 +381,48 @@ public Counter newMetric(Class<?> clazz, String name,
String scope) {
MetricsConnection(String scope, Supplier<ThreadPoolExecutor> batchPool,
Supplier<ThreadPoolExecutor> metaPool) {
this.scope = scope;
+ this.batchPools.add(batchPool);
+ this.metaPools.add(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 (int i = 0; i < batchPools.size(); i++) {
+ ThreadPoolExecutor pool = (ThreadPoolExecutor) ((Supplier)
batchPools.get(i)).get();
Review Comment:
If the list is typed, can this become
```suggestion
for (Supplier<ThreadPoolExecutor> poolSupplier : batchPools) {
ThreadPoolExecutor pool = poolSupplier.get();
```
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetricsConnection.java:
##########
@@ -334,27 +381,48 @@ public Counter newMetric(Class<?> clazz, String name,
String scope) {
MetricsConnection(String scope, Supplier<ThreadPoolExecutor> batchPool,
Supplier<ThreadPoolExecutor> metaPool) {
this.scope = scope;
+ this.batchPools.add(batchPool);
Review Comment:
can `MetricsConnection` ever receive non-null Pools? Having this code may
make a backport to `branch-2` easier... but it seems like it's no longer
relevant in `master`... I wonder if we should be removing the ratios, or just
returning a hardcoded 0/0 ratio.
But probably easier to keep as-is for now in this PR, and for the easier
backports. It just seems weird to me since it looks like the pools are always
null here.
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetricsConnection.java:
##########
@@ -295,8 +337,13 @@ public Counter newMetric(Class<?> clazz, String name,
String scope) {
}
};
+ // List of thread pool per connection of the metrics.
+ private List batchPools = new ArrayList<Supplier>();
+ private List metaPools = new ArrayList<Supplier>();
Review Comment:
Is a typed list possible here?
```suggestion
private List<Supplier<ThreadPoolExecutor>> batchPools = new ArrayList<>();
private List<Supplier<ThreadPoolExecutor>> metaPools = new ArrayList<>();
```
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetricsConnection.java:
##########
@@ -457,6 +525,27 @@ public void incrementServerOverloadedBackoffTime(long
time, TimeUnit timeUnit) {
overloadedBackoffTimer.update(time, timeUnit);
}
+ /** Return the connection count of the metrics within a scope */
+ private long getConnectionCount() {
+ return connectionCount.getCount();
Review Comment:
new test in `TestMetricsConnection` to validate these counts across
connection lifetimes?
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetricsConnection.java:
##########
@@ -54,6 +58,38 @@
@InterfaceAudience.Private
public class MetricsConnection implements StatisticTrackable {
+ static final Map<String, MetricsConnection> METRICS_INSTANCES =
+ new HashMap<String, MetricsConnection>();
+
+ static MetricsConnection getMetricsConnection(final AsyncConnection conn,
+ Supplier<ThreadPoolExecutor> batchPool, Supplier<ThreadPoolExecutor>
metaPool) {
+ String scope = getScope(conn);
+ MetricsConnection metrics;
+ synchronized (METRICS_INSTANCES) {
+ metrics = METRICS_INSTANCES.get(scope);
+ if (metrics == null) {
+ metrics = new MetricsConnection(scope, batchPool, metaPool);
+ METRICS_INSTANCES.put(scope, metrics);
+ } else {
+ metrics.addThreadPools(batchPool, metaPool);
+ }
+ metrics.incrConnectionCount();
+ }
+ return metrics;
+ }
+
+ static void deleteMetricsConnection(final AsyncConnection conn) {
+ String scope = getScope(conn);
+ synchronized (METRICS_INSTANCES) {
+ MetricsConnection metrics = METRICS_INSTANCES.get(scope);
+ metrics.decrConnectionCount();
Review Comment:
It's unexpected, but should we have a null check for `metrics` to avoid any
unexpected NPE, with an error logged?
--
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]
