Repository: pinpoint-apm/pinpoint
Issue: Refactor hbase connection and table/admin factories
Labels: enhancement

#### Description
**Admin** instances are light and not thread-safe, so it would make sense to obtain it whenever it is needed from hbase **Connection** (of which it's creation is a heavy-weight operation but thread-safe).
Since hbase **Table** creation also requires a connection, it would make sense to share a single connection between for **Table** and **Admin** factories.

Issue comments:

Pull request review comments:

Patch that resolved the issue:
diff --git a/collector/src/main/resources/applicationContext-hbase.xml b/collector/src/main/resources/applicationContext-hbase.xml
index 0918686b2431..64f9860757f0 100644
--- a/collector/src/main/resources/applicationContext-hbase.xml
+++ b/collector/src/main/resources/applicationContext-hbase.xml
@@ -45,15 +45,19 @@
         <property name="preStartAllCoreThreads" value="true"/>
     </bean>
 
-    <bean id="connectionFactory" class="com.navercorp.pinpoint.common.hbase.PooledHTableFactory">
+    <bean id="connectionFactory" class="com.navercorp.pinpoint.common.hbase.ConnectionFactoryBean">
         <constructor-arg type="org.apache.hadoop.conf.Configuration" ref="hbaseConfiguration"/>
         <constructor-arg type="java.util.concurrent.ExecutorService" ref="hbaseThreadPool"/>
     </bean>
 
+    <bean id="hbaseTableFactory" class="com.navercorp.pinpoint.common.hbase.HbaseTableFactory">
+        <constructor-arg ref="connectionFactory"/>
+    </bean>
+
     <bean class="org.apache.hadoop.util.ShutdownHookManagerProxy"/>
 
     <bean id="asyncOperation" class="com.navercorp.pinpoint.common.hbase.HBaseAsyncOperationFactory" factory-method="create">
-        <constructor-arg type="org.apache.hadoop.hbase.client.Connection" value="#{connectionFactory.getConnection()}"/>
+        <constructor-arg type="org.apache.hadoop.hbase.client.Connection" ref="connectionFactory"/>
         <constructor-arg type="org.apache.hadoop.conf.Configuration" ref="hbaseConfiguration"/>
     </bean>
 
@@ -63,12 +67,16 @@
 
     <bean id="hbaseTemplate" class="com.navercorp.pinpoint.common.hbase.HbaseTemplate2" destroy-method="destroy">
         <property name="configuration" ref="hbaseConfiguration"/>
-        <property name="tableFactory" ref="connectionFactory"/>
+        <property name="tableFactory" ref="hbaseTableFactory"/>
         <property name="asyncOperation" ref="asyncOperation"/>
     </bean>
 
-    <bean id="hBaseAdminTemplate" class="com.navercorp.pinpoint.common.hbase.HBaseAdminTemplate" destroy-method="close">
-        <constructor-arg ref="hbaseConfiguration" index="0"/>
+    <bean id="hbaseAdminFactory" class="com.navercorp.pinpoint.common.hbase.HbaseAdminFactory">
+        <constructor-arg ref="connectionFactory"/>
+    </bean>
+
+    <bean id="hbaseAdminTemplate" class="com.navercorp.pinpoint.common.hbase.HBaseAdminTemplate">
+        <constructor-arg ref="hbaseAdminFactory"/>
     </bean>
 
     <bean id="applicationTraceIndexDistributor" class="com.sematext.hbase.wd.RowKeyDistributorByHashPrefix">
diff --git a/commons-hbase/src/main/java/com/navercorp/pinpoint/common/hbase/AdminFactory.java b/commons-hbase/src/main/java/com/navercorp/pinpoint/common/hbase/AdminFactory.java
new file mode 100644
index 000000000000..a9785ad374b8
--- /dev/null
+++ b/commons-hbase/src/main/java/com/navercorp/pinpoint/common/hbase/AdminFactory.java
@@ -0,0 +1,30 @@
+/*
+ * Copyright 2018 NAVER Corp.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.navercorp.pinpoint.common.hbase;
+
+import org.apache.hadoop.hbase.client.Admin;
+
+/**
+ * @author HyunGil Jeong
+ */
+public interface AdminFactory {
+
+    Admin getAdmin();
+
+    void releaseAdmin(Admin admin);
+
+}
diff --git a/commons-hbase/src/main/java/com/navercorp/pinpoint/common/hbase/ConnectionFactoryBean.java b/commons-hbase/src/main/java/com/navercorp/pinpoint/common/hbase/ConnectionFactoryBean.java
new file mode 100644
index 000000000000..8f922055b787
--- /dev/null
+++ b/commons-hbase/src/main/java/com/navercorp/pinpoint/common/hbase/ConnectionFactoryBean.java
@@ -0,0 +1,88 @@
+/*
+ * Copyright 2018 NAVER Corp.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.navercorp.pinpoint.common.hbase;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.client.Connection;
+import org.apache.hadoop.hbase.client.ConnectionFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.springframework.beans.factory.DisposableBean;
+import org.springframework.beans.factory.FactoryBean;
+
+import java.io.IOException;
+import java.util.Objects;
+import java.util.concurrent.ExecutorService;
+
+/**
+ * @author HyunGil Jeong
+ */
+public class ConnectionFactoryBean implements FactoryBean<Connection>, DisposableBean {
+
+    private final Logger logger = LoggerFactory.getLogger(this.getClass());
+
+    private final Connection connection;
+
+    public ConnectionFactoryBean(Configuration configuration) {
+        Objects.requireNonNull(configuration, " must not be null");
+        try {
+            connection = ConnectionFactory.createConnection(configuration);
+        } catch (IOException e) {
+            throw new HbaseSystemException(e);
+        }
+    }
+
+    public ConnectionFactoryBean(Configuration configuration, ExecutorService executorService) {
+        Objects.requireNonNull(configuration, "configuration must not be null");
+        Objects.requireNonNull(executorService, "executorService must not be null");
+        try {
+            connection = ConnectionFactory.createConnection(configuration, executorService);
+        } catch (IOException e) {
+            throw new HbaseSystemException(e);
+        }
+    }
+
+    @Override
+    public Connection getObject() throws Exception {
+        return connection;
+    }
+
+    @Override
+    public Class<?> getObjectType() {
+        if (connection == null) {
+            return Connection.class;
+        }
+        return connection.getClass();
+    }
+
+    @Override
+    public boolean isSingleton() {
+        return true;
+    }
+
+    @Override
+    public void destroy() throws Exception {
+        logger.info("Hbase Connection destroy()");
+        if (connection != null) {
+            try {
+                connection.close();
+            } catch (IOException e) {
+                logger.warn("Hbase Connection.close() error: " + e.getMessage(), e);
+            }
+        }
+    }
+}
diff --git a/commons-hbase/src/main/java/com/navercorp/pinpoint/common/hbase/HBaseAdminTemplate.java b/commons-hbase/src/main/java/com/navercorp/pinpoint/common/hbase/HBaseAdminTemplate.java
index 1294c329cc54..da4d5b925337 100644
--- a/commons-hbase/src/main/java/com/navercorp/pinpoint/common/hbase/HBaseAdminTemplate.java
+++ b/commons-hbase/src/main/java/com/navercorp/pinpoint/common/hbase/HBaseAdminTemplate.java
@@ -17,79 +17,76 @@
 package com.navercorp.pinpoint.common.hbase;
 
 import java.io.IOException;
+import java.util.Objects;
 
-import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.hbase.HTableDescriptor;
 import org.apache.hadoop.hbase.TableName;
 import org.apache.hadoop.hbase.client.Admin;
-import org.apache.hadoop.hbase.client.Connection;
-import org.apache.hadoop.hbase.client.ConnectionFactory;
 
 /**
  * @author emeroad
+ * @author HyunGil Jeong
  */
 public class HBaseAdminTemplate {
 
-    private final Admin admin;
-    private final Connection connection;
+    private final AdminFactory adminFactory;
 
-    public HBaseAdminTemplate(Configuration configuration) {
-        try {
-            connection = ConnectionFactory.createConnection(configuration);
-            admin = connection.getAdmin();
-        } catch (Exception e) {
-            throw new HbaseSystemException(e);
-        }
+    public HBaseAdminTemplate(AdminFactory adminFactory) {
+        this.adminFactory = Objects.requireNonNull(adminFactory, "adminFactory must not be null");
     }
 
     public boolean createTableIfNotExist(HTableDescriptor htd) {
+        Admin admin = adminFactory.getAdmin();
         try {
-            if (!admin.tableExists(htd.getTableName())) {
-                this.admin.createTable(htd);
+            TableName tableName = htd.getTableName();
+            if (!admin.tableExists(tableName)) {
+                admin.createTable(htd);
                 return true;
             }
             return false;
         } catch (IOException e) {
             throw new HbaseSystemException(e);
+        } finally {
+            adminFactory.releaseAdmin(admin);
         }
     }
 
     public boolean tableExists(TableName tableName) {
+        Admin admin = adminFactory.getAdmin();
         try {
             return admin.tableExists(tableName);
         } catch (IOException e) {
             throw new HbaseSystemException(e);
+        } finally {
+            adminFactory.releaseAdmin(admin);
         }
     }
 
     public boolean dropTableIfExist(TableName tableName) {
+        Admin admin = adminFactory.getAdmin();
         try {
             if (admin.tableExists(tableName)) {
-                this.admin.disableTable(tableName);
-                this.admin.deleteTable(tableName);
+                admin.disableTable(tableName);
+                admin.deleteTable(tableName);
                 return true;
             }
             return false;
         } catch (IOException e) {
             throw new HbaseSystemException(e);
+        } finally {
+            adminFactory.releaseAdmin(admin);
         }
     }
 
     public void dropTable(TableName tableName) {
+        Admin admin = adminFactory.getAdmin();
         try {
-            this.admin.disableTable(tableName);
-            this.admin.deleteTable(tableName);
-        } catch (IOException e) {
-            throw new HbaseSystemException(e);
-        }
-    }
-
-    public void close() {
-        try {
-            this.admin.close();
-            this.connection.close();
+            admin.disableTable(tableName);
+            admin.deleteTable(tableName);
         } catch (IOException e) {
             throw new HbaseSystemException(e);
+        } finally {
+            adminFactory.releaseAdmin(admin);
         }
     }
 }
diff --git a/commons-hbase/src/main/java/com/navercorp/pinpoint/common/hbase/HbaseAdminFactory.java b/commons-hbase/src/main/java/com/navercorp/pinpoint/common/hbase/HbaseAdminFactory.java
new file mode 100644
index 000000000000..cdb2b87c779f
--- /dev/null
+++ b/commons-hbase/src/main/java/com/navercorp/pinpoint/common/hbase/HbaseAdminFactory.java
@@ -0,0 +1,56 @@
+/*
+ * Copyright 2018 NAVER Corp.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.navercorp.pinpoint.common.hbase;
+
+import org.apache.hadoop.hbase.client.Admin;
+import org.apache.hadoop.hbase.client.Connection;
+
+import java.io.IOException;
+import java.util.Objects;
+
+/**
+ * @author HyunGil Jeong
+ */
+public class HbaseAdminFactory implements AdminFactory {
+
+    private final Connection connection;
+
+    public HbaseAdminFactory(Connection connection) {
+        this.connection = Objects.requireNonNull(connection, "connection must not be null");
+    }
+
+    @Override
+    public Admin getAdmin() {
+        try {
+            return connection.getAdmin();
+        } catch (IOException e) {
+            throw new HbaseSystemException(e);
+        }
+    }
+
+    @Override
+    public void releaseAdmin(Admin admin) {
+        if (admin == null) {
+            return;
+        }
+        try {
+            admin.close();
+        } catch (IOException e) {
+            throw new HbaseSystemException(e);
+        }
+    }
+}
diff --git a/commons-hbase/src/main/java/com/navercorp/pinpoint/common/hbase/PooledHTableFactory.java b/commons-hbase/src/main/java/com/navercorp/pinpoint/common/hbase/HbaseTableFactory.java
similarity index 55%
rename from commons-hbase/src/main/java/com/navercorp/pinpoint/common/hbase/PooledHTableFactory.java
rename to commons-hbase/src/main/java/com/navercorp/pinpoint/common/hbase/HbaseTableFactory.java
index 5cb3e591ddf3..cecd54e18daa 100644
--- a/commons-hbase/src/main/java/com/navercorp/pinpoint/common/hbase/PooledHTableFactory.java
+++ b/commons-hbase/src/main/java/com/navercorp/pinpoint/common/hbase/HbaseTableFactory.java
@@ -16,14 +16,9 @@
 
 package com.navercorp.pinpoint.common.hbase;
 
-import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.hbase.TableName;
 import org.apache.hadoop.hbase.client.Connection;
-import org.apache.hadoop.hbase.client.ConnectionFactory;
 import org.apache.hadoop.hbase.client.Table;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-import org.springframework.beans.factory.DisposableBean;
 
 import java.io.IOException;
 import java.util.Objects;
@@ -32,35 +27,21 @@
 /**
  * HTableInterfaceFactory based on HTablePool.
  * @author emeroad
- * @autor minwoo.jung
+ * @author minwoo.jung
+ * @author HyunGil Jeong
  */
-public class PooledHTableFactory implements TableFactory, DisposableBean {
+public class HbaseTableFactory implements TableFactory {
 
-    private final Logger logger = LoggerFactory.getLogger(this.getClass());
-
-    private final ExecutorService executor;
     private final Connection connection;
 
-    public PooledHTableFactory(Configuration config, ExecutorService executor) {
-        Objects.requireNonNull(config, "config must not be null");
-        this.executor = Objects.requireNonNull(executor, "executor must not be null");
-
-        try {
-            this.connection = ConnectionFactory.createConnection(config, executor);
-        } catch (IOException e) {
-            throw new HbaseSystemException(e);
-        }
-    }
-
-    public Connection getConnection() {
-        return connection;
+    public HbaseTableFactory(Connection connection) {
+        this.connection = Objects.requireNonNull(connection, "connection must not be null");
     }
 
-
     @Override
     public Table getTable(TableName tableName) {
         try {
-            return connection.getTable(tableName, executor);
+            return connection.getTable(tableName);
         } catch (IOException e) {
             throw new HbaseSystemException(e);
         }
@@ -87,18 +68,4 @@ public void releaseTable(Table table) {
             throw new HbaseSystemException(ex);
         }
     }
-
-
-    @Override
-    public void destroy() throws Exception {
-        logger.info("PooledHTableFactory.destroy()");
-        
-        if (connection != null) {
-            try {
-                this.connection.close();
-            } catch (IOException ex) {
-                logger.warn("Connection.close() error:" + ex.getMessage(), ex);
-            }
-        }
-    }
 }
diff --git a/flink/src/main/resources/applicationContext-hbase.xml b/flink/src/main/resources/applicationContext-hbase.xml
index 9296393004b6..1a7d83b447b9 100644
--- a/flink/src/main/resources/applicationContext-hbase.xml
+++ b/flink/src/main/resources/applicationContext-hbase.xml
@@ -57,13 +57,17 @@
         <property name="preStartAllCoreThreads" value="true"/>
     </bean>
 
-    <bean id="connectionFactory" class="com.navercorp.pinpoint.common.hbase.PooledHTableFactory">
+    <bean id="connectionFactory" class="com.navercorp.pinpoint.common.hbase.ConnectionFactoryBean">
         <constructor-arg type="org.apache.hadoop.conf.Configuration" ref="hbaseConfiguration"/>
         <constructor-arg type="java.util.concurrent.ExecutorService" ref="hbaseThreadPool"/>
     </bean>
 
+    <bean id="hbaseTableFactory" class="com.navercorp.pinpoint.common.hbase.HbaseTableFactory">
+        <constructor-arg ref="connectionFactory"/>
+    </bean>
+
     <bean id="asyncOperation" class="com.navercorp.pinpoint.common.hbase.HBaseAsyncOperationFactory" factory-method="create">
-        <constructor-arg value="#{connectionFactory.getConnection()}"/>
+        <constructor-arg ref="connectionFactory"/>
         <constructor-arg ref="hbaseConfiguration"/>
     </bean>
 
@@ -73,12 +77,16 @@
 
     <bean id="hbaseTemplate" class="com.navercorp.pinpoint.common.hbase.HbaseTemplate2" destroy-method="destroy">
         <property name="configuration" ref="hbaseConfiguration"/>
-        <property name="tableFactory" ref="connectionFactory"/>
+        <property name="tableFactory" ref="hbaseTableFactory"/>
         <property name="asyncOperation" ref="asyncOperation"/>
     </bean>
 
-    <bean id="hBaseAdminTemplate" class="com.navercorp.pinpoint.common.hbase.HBaseAdminTemplate" destroy-method="close">
-        <constructor-arg ref="hbaseConfiguration" index="0"></constructor-arg>
+    <bean id="hbaseAdminFactory" class="com.navercorp.pinpoint.common.hbase.HbaseAdminFactory">
+        <constructor-arg ref="connectionFactory"/>
+    </bean>
+
+    <bean id="hbaseAdminTemplate" class="com.navercorp.pinpoint.common.hbase.HBaseAdminTemplate">
+        <constructor-arg ref="hbaseAdminFactory"/>
     </bean>
 
     <bean id="tableNameProvider" class="com.navercorp.pinpoint.common.hbase.HbaseTableNameProvider">
diff --git a/web/src/main/resources/applicationContext-hbase.xml b/web/src/main/resources/applicationContext-hbase.xml
index 798e3111b9fe..971906d9be0a 100644
--- a/web/src/main/resources/applicationContext-hbase.xml
+++ b/web/src/main/resources/applicationContext-hbase.xml
@@ -44,21 +44,29 @@
         <property name="preStartAllCoreThreads" value="true"/>
     </bean>
 
-    <bean id="connectionFactory" class="com.navercorp.pinpoint.common.hbase.PooledHTableFactory">
-        <constructor-arg ref="hbaseConfiguration"/>
-        <constructor-arg ref="hbaseThreadPool"/>
+    <bean id="connectionFactory" class="com.navercorp.pinpoint.common.hbase.ConnectionFactoryBean">
+        <constructor-arg type="org.apache.hadoop.conf.Configuration" ref="hbaseConfiguration"/>
+        <constructor-arg type="java.util.concurrent.ExecutorService" ref="hbaseThreadPool"/>
+    </bean>
+
+    <bean id="hbaseTableFactory" class="com.navercorp.pinpoint.common.hbase.HbaseTableFactory">
+        <constructor-arg ref="connectionFactory"/>
     </bean>
 
     <bean id="hbaseTemplate" class="com.navercorp.pinpoint.common.hbase.HbaseTemplate2">
         <property name="configuration" ref="hbaseConfiguration"/>
-        <property name="tableFactory" ref="connectionFactory"/>
+        <property name="tableFactory" ref="hbaseTableFactory"/>
         <property name="enableParallelScan" value="${hbase.client.parallel.scan.enable:false}"/>
         <property name="maxThreads" value="${hbase.client.parallel.scan.maxthreads:16}"/>
         <property name="maxThreadsPerParallelScan" value="${hbase.client.parallel.scan.maxthreadsperscan:4}"/>
     </bean>
-    
-    <bean id="hBaseAdminTemplate" class="com.navercorp.pinpoint.common.hbase.HBaseAdminTemplate" destroy-method="close">
-        <constructor-arg ref="hbaseConfiguration" index="0"/>
+
+    <bean id="hbaseAdminFactory" class="com.navercorp.pinpoint.common.hbase.HbaseAdminFactory">
+        <constructor-arg ref="connectionFactory"/>
+    </bean>
+
+    <bean id="hbaseAdminTemplate" class="com.navercorp.pinpoint.common.hbase.HBaseAdminTemplate">
+        <constructor-arg ref="hbaseAdminFactory"/>
     </bean>
 
     <bean class="org.apache.hadoop.util.ShutdownHookManagerProxy"/>


Test patch:
diff --git a/commons-hbase/src/test/java/com/navercorp/pinpoint/common/hbase/HbaseTemplate2IT.java b/commons-hbase/src/test/java/com/navercorp/pinpoint/common/hbase/HbaseTemplate2IT.java
index f20a2d159310..9ad281f14e15 100644
--- a/commons-hbase/src/test/java/com/navercorp/pinpoint/common/hbase/HbaseTemplate2IT.java
+++ b/commons-hbase/src/test/java/com/navercorp/pinpoint/common/hbase/HbaseTemplate2IT.java
@@ -18,13 +18,13 @@
 
 import java.io.IOException;
 import java.util.Properties;
-import java.util.concurrent.ExecutorService;
-import java.util.concurrent.Executors;
 
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.hbase.HBaseConfiguration;
 import org.apache.hadoop.hbase.TableName;
 import org.apache.hadoop.hbase.TableNotFoundException;
+import org.apache.hadoop.hbase.client.Connection;
+import org.apache.hadoop.hbase.client.ConnectionFactory;
 import org.apache.hadoop.hbase.client.RetriesExhaustedWithDetailsException;
 import org.junit.AfterClass;
 import org.junit.Assert;
@@ -40,7 +40,7 @@
  * @author minwoo.jung
  */
 public class HbaseTemplate2IT {
-    private static ExecutorService executorService;
+    private static Connection connection;
     private static HbaseTemplate2 hbaseTemplate2;
 
     @BeforeClass
@@ -51,10 +51,10 @@ public static void beforeClass() throws IOException {
         cfg.set("hbase.zookeeper.quorum", properties.getProperty("hbase.client.host"));
         cfg.set("hbase.zookeeper.property.clientPort", properties.getProperty("hbase.client.port"));
 
-        executorService = Executors.newFixedThreadPool(10);
+        connection = ConnectionFactory.createConnection(cfg);
         hbaseTemplate2 = new HbaseTemplate2();
         hbaseTemplate2.setConfiguration(cfg);
-        hbaseTemplate2.setTableFactory(new PooledHTableFactory(cfg, executorService));
+        hbaseTemplate2.setTableFactory(new HbaseTableFactory(connection));
         hbaseTemplate2.afterPropertiesSet();
     }
 
@@ -63,8 +63,8 @@ public static void afterClass() throws Exception {
         if (hbaseTemplate2 != null) {
             hbaseTemplate2.destroy();
         }
-        if (executorService != null) {
-            executorService.shutdown();
+        if (connection != null) {
+            connection.close();
         }
     }