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

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

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



##########
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:
       This is perfect, but for a while I got confused with `connection` being Connection and `this.connection` being AtomicReference. 
   Can we rename `connection` to `connectionRef` to indicate AtomicReference?




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