HBASE-28669: fix leaks a connection to ZooKeeper #6147
Merged
Conversation
This file contains hidden or 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. |
Contributor
Author
|
When I turn on replication and create a peer, if a RegionServer goes down, after recovery, I delete a peer, but the connection between the peer and zookeeper will not be disconnected. |
Contributor
Author
|
When I checked the code, I found that when RecoveredReplicationSource executed tryFinish, only ReplicationSource was deleted from oldsources, but terminate was not executed, resulting in connection leaks. |
Apache9
reviewed
Aug 12, 2024
| () -> { | ||
| if (workerThreads.isEmpty()) { | ||
| this.getSourceMetrics().clear(); | ||
| this.terminate("Finished recovering queue"); |
Contributor
There was a problem hiding this comment.
I think here we just need to call replicationEndpoint.stop? The terminate method is designed for terminate a replication source when removing a peer or refreshing a source when there are still data need to be replicated in the source, so it contains a lot of unnecessary logics when we just want to remove a finished recovered source.
Contributor
Author
There was a problem hiding this comment.
Yes, calling replicationEndpoint.stop can solve this bug. Don't the workerThreads created during the initialization of RecoveredReplicationSource need to be stopped? I think the context simply removes it from the map.
Contributor
There was a problem hiding this comment.
ReplicationSourceShipper is just a thread, it will quit after we returning from the run method, so we do not need to stop it again. And even if you call terminate method here, there is no way to close these threads as there is a workerThreads.isEmpty check above...
Contributor
Author
There was a problem hiding this comment.
OK, thanks for your help. It is more reasonable to call replicationEndpoint.stop here without call terminate.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
approved these changes
Sep 4, 2024
Apache9
pushed a commit
that referenced
this pull request
Sep 4, 2024
…ks a connection to ZooKeeper (#6147) Co-authored-by: rodenli <[email protected]> Signed-off-by: Duo Zhang <[email protected]> (cherry picked from commit 73651dc)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
fix HBASE-28669