d-c-manning commented on code in PR #4874:
URL: https://github.com/apache/hbase/pull/4874#discussion_r1025888411
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetricsConnection.java:
##########
@@ -54,6 +58,37 @@
@InterfaceAudience.Private
public class MetricsConnection implements StatisticTrackable {
+ static final Map<String, MetricsConnection> METRICS_INSTANCES =
+ new HashMap<String, MetricsConnection>();
+
+ static MetricsConnection getMetricsConnection(final String scope,
Review Comment:
The comment on line 55-56 about "be sure to call shutdown()" could be
removed, and replaced by "Calling `getMetricsConnection` implicity creates and
"starts" instances of these classes; be sure to call `deleteMetricsConnection`
to terminate the thread pools they allocate. You could also add a comment about
the ref counting model now being used for these objects.
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetricsConnection.java:
##########
@@ -331,30 +371,53 @@ 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;
+ this.batchPools.add(batchPool);
+ this.metaPools.add(metaPool);
Review Comment:
nit: could reuse the same method used in the get call
```suggestion
addThreadPools(batchPool, metaPool);
```
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnectionImpl.java:
##########
@@ -118,6 +118,7 @@ public class AsyncConnectionImpl implements AsyncConnection
{
private final AtomicBoolean closed = new AtomicBoolean(false);
+ private final String clusterId;
Review Comment:
Instead of storing `clusterId` to recompute the `scope` in `close()`, should
we store the `scope` we receive from `getScope` in the constructor?
```suggestion
private final String metricsScope;
```
##########
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:
yeah I think if we change it, it should be outside the scope of this PR for
sure.
--
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]
