You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by GitBox <gi...@apache.org> on 2020/08/03 03:47:50 UTC
[GitHub] [hbase] busbey opened a new pull request #2188: HBASE-24805 HBaseTestingUtility.getConnection should be threadsafe (branch-1)
busbey opened a new pull request #2188:
URL: https://github.com/apache/hbase/pull/2188
* refactor how we use connection to rely on the access method
* refactor initialization and cleanup of the shared connection
* incompatibly change HCTU's Configuration member variable to be final so it can be safely accessed from multiple threads.
Closes #2180
adapted for jdk7
(cherry picked from commit 86ebbdd8a2df89de37c2c3bd50e64292eaf28b11)
(cherry picked from commit 0806349adab338330428c900588234d7f6fcfcc2)
----------------------------------------------------------------
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
[GitHub] [hbase] busbey commented on a change in pull request #2188: HBASE-24805 HBaseTestingUtility.getConnection should be threadsafe (branch-1)
Posted by 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
[GitHub] [hbase] virajjasani commented on a change in pull request #2188: HBASE-24805 HBaseTestingUtility.getConnection should be threadsafe (branch-1)
Posted by 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 logic 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
[GitHub] [hbase] virajjasani commented on a change in pull request #2188: HBASE-24805 HBaseTestingUtility.getConnection should be threadsafe (branch-1)
Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #2188:
URL: https://github.com/apache/hbase/pull/2188#discussion_r464833942
##########
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:
I think branch-2+ seem good because names are different already 👍
----------------------------------------------------------------
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
[GitHub] [hbase] virajjasani commented on a change in pull request #2188: HBASE-24805 HBaseTestingUtility.getConnection should be threadsafe (branch-1)
Posted by 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
[GitHub] [hbase] busbey closed pull request #2188: HBASE-24805 HBaseTestingUtility.getConnection should be threadsafe (branch-1)
Posted by GitBox <gi...@apache.org>.
busbey closed pull request #2188:
URL: https://github.com/apache/hbase/pull/2188
----------------------------------------------------------------
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