HBASE-28146: Make ServerManager rsAdmins map thread safe #5461
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
This problem only affects 2.x? Master and branch-3 are safe? |
It looks like master was fixed via your commit 5d872d3. Should we make a similar change here? Otherwise if we intend to just fix this bug here, then looking at ServerManager.getRsAdmin, it is not thread safe. We should probably use computeIfAbsent if the get returns null. |
|
Ah, OK, we change to use async connection for master and branch-3... |
Apache9
reviewed
Oct 12, 2023
hbase-server/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java
Outdated
Show resolved
Hide resolved
Apache9
reviewed
Oct 12, 2023
| try { | ||
| return this.connection.getAdmin(server); | ||
| } catch (IOException e) { | ||
| throw new RuntimeException(e); |
There was a problem hiding this comment.
We should throw this IOException out? This is a problem of computeIfAbsent, as the functional interface does not throw IOException...
There was a problem hiding this comment.
Yeah, good point
There was a problem hiding this comment.
As written, it seems we need some synchronization. A concurrent map is pointless if you do unsyncrhonized writes on it. You could use ConcurrentMapUtils.computeIfAbsentEx to get around the exception issue.
That said, looking at the impls of getRSRpcServices and getAdmin, I don't think we really need to cache these at all. getRSRpcServices directly returns an instance variable. getAdmin already does it's own caching.
So it might make sense to simply remove the rsAdmins map. @Apache9 what do you think?
There was a problem hiding this comment.
computeIfAbsentEx seems like a good idea here. I don't have an informed opinion regarding whether the caching is really necessary, but I'm happy to rip it out if we think that's worth doing now
There was a problem hiding this comment.
Ah, checked the code, as @bbeaudreault mentioned, we already have a cache in ConnectionImplementation,
So I think we can just remove the cache here.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
| admin = this.connection.getAdmin(sn); | ||
| } | ||
| this.rsAdmins.put(sn, admin); | ||
| LOG.debug("New admin connection to " + sn.toString()); |
There was a problem hiding this comment.
can you change this to use a {}, and just pass in sn instead of sn.toString()? Not sure how useful the log is now, we could remove it maybe, but at the least we should avoid the minor bad logging practice
There was a problem hiding this comment.
nice idea. agreed it could be useless, but I want to keep this log in so that we've got some ability to investigate if the removal of caching causes more work than we've anticipated here
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
bbeaudreault
approved these changes
Oct 21, 2023
Apache9
approved these changes
Oct 21, 2023
bbeaudreault
pushed a commit
to HubSpot/hbase
that referenced
this pull request
Oct 23, 2023
… safe (apache#5461) Co-authored-by: Ray Mattingly <rmattingly@hubspot.com> Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org>
Apache9
pushed a commit
that referenced
this pull request
Oct 23, 2023
Co-authored-by: Ray Mattingly <rmattingly@hubspot.com> Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org> (cherry picked from commit 1641a4a)
Apache9
pushed a commit
that referenced
this pull request
Oct 23, 2023
Co-authored-by: Ray Mattingly <rmattingly@hubspot.com> Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org> (cherry picked from commit 1641a4a)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
On 2.x the ServerManager registers admins in a HashMap. This can result in thread safety issues — we recently observed an exception which caused a region to be indefinitely stuck in transition until we could manually intervene. We saw the following exception in the HMaster logs:
This error is not a particularly clear indication of concurrency issues, but that's more an issue with HashMap error handling. Here's a SO thread which discusses the error and why it occurs. This is a good explanation imo.
cc @bbeaudreault @hgromer @eab148 @bozzkar