[GitHub] [hbase] busbey commented on a change in pull request #2188: HBASE-24805 HBaseTestingUtility.getConnection should be threadsafe (branch-1)

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

busbey commented on a change in pull request #2188:
URL: https://github.com/apache/hbase/pull/2188#discussion_r464567541



##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java
##########
@@ -3020,16 +3011,26 @@ public HBaseCluster getHBaseClusterInterface() {
   }
 
   /**
-   * Get a Connection to the cluster.
-   * Not thread-safe (This class needs a lot of work to make it thread-safe).
+   * Get a shared Connection to the cluster.
+   * this method is threadsafe.
    * @return A Connection that can be shared. Don't close. Will be closed on shutdown of cluster.
    * @throws IOException
    */
   public Connection getConnection() throws IOException {
-    if (this.connection == null) {
-      this.connection = ConnectionFactory.createConnection(this.conf);
+    Connection connection = this.connection.get();
+    while (connection == null) {
+      connection = ConnectionFactory.createConnection(this.conf);
+      if (! this.connection.compareAndSet(null, connection)) {
+        try {
+          connection.close();
+        } catch (IOException exception) {
+          LOG.debug("Ignored failure while closing connection on contended connection creation.",
+              exception);
+        }
+        connection = this.connection.get();

Review comment:
       sure! should I update master and branches-2 to similarly use `asyncConnectionRef` instead of `asyncConnection`? Or less confusing there because the instance and local names are already different?




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org