New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HBASE-24805 HBaseTestingUtility.getConnection should be threadsafe (branch-1) #2188
Conversation
* 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 apache#2180 adapted for jdk7 (cherry picked from commit 86ebbdd) (cherry picked from commit 0806349)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a minor nit, +1 otherwise
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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think branch-2+ seem good because names are different already 👍
…to the shared connection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
* 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 #2188 adapted for jdk7 Signed-off-by: Viraj Jasani <vjasani@apache.org> (cherry picked from commit 86ebbdd) (cherry picked from commit 0806349)
Closes #2180
adapted for jdk7
(cherry picked from commit 86ebbdd)
(cherry picked from commit 0806349)