apurtell commented on code in PR #4874:
URL: https://github.com/apache/hbase/pull/4874#discussion_r1020585404
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnection.java:
##########
@@ -34,6 +34,16 @@
@InterfaceAudience.Public
public interface AsyncConnection extends Closeable {
+ /**
+ * Returns the clusterId of this connection.
+ */
+ String getClusterId2();
Review Comment:
This is not a good method name for a public API. Please find a better one.
##########
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;
Review Comment:
Comment here to explain what is being measured, the rationale for it, please.
##########
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) {
Review Comment:
Consider a synchronized or concurrent map type instead.
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionFactory.java:
##########
@@ -216,6 +216,37 @@ public static Connection createConnection(Configuration
conf, User user) throws
*/
public static Connection createConnection(Configuration conf,
ExecutorService pool,
final User user) throws IOException {
+ return createConnection(conf, pool, user, null);
+ }
+
+ /**
+ * Create a new Connection instance using the passed <code>conf</code>
instance. Connection
+ * encapsulates all housekeeping for a connection to the cluster. All tables
and interfaces
+ * created from returned connection share zookeeper connection, meta cache,
and connections to
+ * region servers and masters. <br>
+ * The caller is responsible for calling {@link Connection#close()} on the
returned connection
+ * instance. Typical usage:
+ *
+ * <pre>
+ * Connection connection = ConnectionFactory.createConnection(conf);
+ * Table table = connection.getTable(TableName.valueOf("table1"));
+ * try {
+ * table.get(...);
+ * ...
+ * } finally {
+ * table.close();
+ * connection.close();
+ * }
+ * </pre>
+ *
+ * @param conf configuration
+ * @param user the user the connection is for
+ * @param pool the thread pool to use for batch operations
+ * @param identity the identity of the metrics for this connection.
Review Comment:
Same ... "scope" instead of "identity"?
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnection.java:
##########
@@ -34,6 +34,16 @@
@InterfaceAudience.Public
public interface AsyncConnection extends Closeable {
+ /**
+ * Returns the clusterId of this connection.
+ */
+ String getClusterId2();
+
+ /**
+ * Returns this connection identity.
+ */
+ String getIdentity();
Review Comment:
Should we be using "scope" instead of "identity"?
--
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]
