[GitHub] [hbase] bbeaudreault commented on a change in pull request #4285: HBASE-26891 Make MetricsConnection scope configurable

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

bbeaudreault commented on a change in pull request #4285:
URL: https://github.com/apache/hbase/pull/4285#discussion_r841107787



##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnectionImpl.java
##########
@@ -132,7 +133,8 @@ public AsyncConnectionImpl(Configuration conf, ConnectionRegistry registry, Stri
     this.connConf = new AsyncConnectionConfiguration(conf);
     this.registry = registry;
     if (conf.getBoolean(CLIENT_SIDE_METRICS_ENABLED_KEY, false)) {
-      this.metrics = Optional.of(new MetricsConnection(this.toString(), () -> null, () -> null));
+      String scope = conf.get(METRICS_SCOPE_KEY, clusterId + "-" + hashCode());

Review comment:
       I personally think this.toString() is a relatively bad default because it doesn't help identify at all. If you have multiple connections going, you'll just see 2 beans: `AsyncConnectionImpl@23fd322` and `AsyncConnectionImpl@abcd123`. What do those signify? Impossible to really know.
   
   The one benefit of that default is that the hashCode ensures that it will indeed be 2 distinct beans in jmx.
   
   The change I propose still may not help if you have 2 connections to the same cluster -- for clusterId `foo` you might then see `foo-23fd322` and `foo-abcd123`.  In that case, if you care, you should customize the scope.
   
   But if you have two connections to different clusters, it will be really helpful i think -- for clusterIds `foo` and `bar` you'd then see `foo-23fd322` and `bar-abcd123`.
   
   The only problem i see iwth this change is im not sure if it's a compatibility issue?




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