Skip to content

HBASE-28669: fix leaks a connection to ZooKeeper #6147

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.

Sign up for GitHub

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

Merged
Apache9 merged 2 commits into from
Sep 4, 2024
Merged

HBASE-28669: fix leaks a connection to ZooKeeper #6147

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
83c8878
HBASE-28669: fix leaks a connection to ZooKeeper
Aug 9, 2024
7df5dc8
HBASE-28669: call only stop without calling terminate
Aug 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ...ain/java/org/apache/hadoop/hbase/replication/regionserver/RecoveredReplicationSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ protected RecoveredReplicationSourceShipper createNewShipper(String walGroupId,
() -> {
if (workerThreads.isEmpty()) {
this.getSourceMetrics().clear();
this.terminate("Finished recovering queue");
Copy link
Contributor

@Apache9 Apache9 Aug 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@wdzaslzy wdzaslzy Aug 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@Apache9 Apache9 Aug 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@wdzaslzy wdzaslzy Aug 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, thanks for your help. It is more reasonable to call replicationEndpoint.stop here without call terminate.

manager.finishRecoveredSource(this);
}
});
Expand Down