Skip to content
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

Jump to bottom

Fix autoRecover memory leak. #3361

Merged
merged 2 commits into from
Jul 26, 2022
Merged

Fix autoRecover memory leak. #3361

merged 2 commits into from
Jul 26, 2022

Conversation

horizonzy
Copy link
Member

Descriptions of the changes in this PR:
fixes-#3360

@horizonzy horizonzy force-pushed the fix-auto-recover-memory-leak branch from c727699 to 4cd2d4e Compare June 25, 2022 08:30
hangc0276
hangc0276 reviewed Jun 26, 2022
View reviewed changes
Copy link
Contributor

@hangc0276 hangc0276 left a comment

Choose a reason for hiding this comment

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

We need to close the ledger handle in Auditor#checkAllLedgers() to unregister the listener.

bookkeeper/bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/Auditor.java

Lines 1333 to 1345 in 677ccec

localAdmin.asyncOpenLedgerNoRecovery(ledgerId, (rc, lh, ctx) -> {
openLedgerNoRecoverySemaphore.release();
if (Code.OK == rc) {
checker.checkLedger(lh,
// the ledger handle will be closed after checkLedger is done.
new ProcessLostFragmentsCb(lh, callback),
conf.getAuditorLedgerVerificationPercentage());
// we collect the following stats to get a measure of the
// distribution of a single ledger within the bk cluster
// the higher the number of fragments/bookies, the more distributed it is
numFragmentsPerLedger.registerSuccessfulValue(lh.getNumFragments());
numBookiesPerLedger.registerSuccessfulValue(lh.getNumBookies());
numLedgersChecked.inc();

@horizonzy
Copy link
Member Author

We need to close the ledger handle in Auditor#checkAllLedgers() to unregister the listener.

bookkeeper/bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/Auditor.java

Lines 1333 to 1345 in 677ccec

localAdmin.asyncOpenLedgerNoRecovery(ledgerId, (rc, lh, ctx) -> {
openLedgerNoRecoverySemaphore.release();
if (Code.OK == rc) {
checker.checkLedger(lh,
// the ledger handle will be closed after checkLedger is done.
new ProcessLostFragmentsCb(lh, callback),
conf.getAuditorLedgerVerificationPercentage());
// we collect the following stats to get a measure of the
// distribution of a single ledger within the bk cluster
// the higher the number of fragments/bookies, the more distributed it is
numFragmentsPerLedger.registerSuccessfulValue(lh.getNumFragments());
numBookiesPerLedger.registerSuccessfulValue(lh.getNumBookies());
numLedgersChecked.inc();

In ProcessLostFragmentsCb, it will close it.

zymap
zymap reviewed Jun 27, 2022
View reviewed changes
bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/AbstractZkLedgerManagerTest.java Outdated
// unregister the listener
ledgerManager.unregisterLedgerMetadataListener(ledgerId, listener);
assertFalse(ledgerManager.listeners.containsKey(ledgerId));
assertFalse(watchers.containsKey(ledgerStr));
verify(mockZk, times(1)).removeWatches(any(String.class),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
verify(mockZk, times(1)).removeWatches(any(String.class),
verify(mockZk, times(1)).removeWatches(eq(getLedgerPath(ledgerId)),

Copy link
Member Author

Choose a reason for hiding this comment

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

good suggestion. 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

good idea, 👍

@zymap zymap added this to the 4.16.0 milestone Jun 27, 2022
hangc0276
hangc0276 approved these changes Jun 30, 2022
View reviewed changes
Copy link
Contributor

@hangc0276 hangc0276 left a comment

Choose a reason for hiding this comment

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

Good job!

zymap
zymap approved these changes Jun 30, 2022
View reviewed changes
Copy link
Member

@zymap zymap left a comment

Choose a reason for hiding this comment

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

LGTM

@zymap
Copy link
Member

zymap commented Jul 6, 2022

@eolivelli @dlg99 Would you like to help to review this PR? Thanks!

eolivelli
eolivelli requested changes Jul 7, 2022
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

good catch !

@dlg99 this may explain some problems you have seen in production

bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java

@Override
public void run() {
Set<LedgerMetadataListener> listeners = AbstractZkLedgerManager.this.listeners.get(ledgerId);
Copy link
Contributor

Choose a reason for hiding this comment

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

this is running in the same thread of "unregisterLedgerMetadataListener"

why do you need a inner class ? can't we simply code this as a regular Java method ?

private void cancelLedgerWatchers(long ledgerId) {
.... 
}

and call it from unregisterLedgerMetadataListener ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Follow the origin code style. See ReadLedgerMetadataTask.

@hangc0276 hangc0276 mentioned this pull request Jul 20, 2022
Closed
11 tasks
@zymap
Copy link
Member

zymap commented Jul 26, 2022

@eolivelli Could you please take another look?

eolivelli
eolivelli approved these changes Jul 26, 2022
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

LGTM

@eolivelli eolivelli merged commit da1b29a into apache:master Jul 26, 2022
@eolivelli
Copy link
Contributor

@hangc0276 please cherry pick to active branches (4.14, 4.15)

@hangc0276
Copy link
Contributor

@hangc0276 please cherry pick to active branches (4.14, 4.15)

@eolivelli sure.

zymap pushed a commit that referenced this pull request Aug 2, 2022
@horizonzy @zymap
Fix autoRecover memory leak. (#3361)
855cf9b 
(cherry picked from commit da1b29a)
@RaulGracia RaulGracia mentioned this pull request Sep 2, 2022
Closed
hangc0276 pushed a commit to hangc0276/bookkeeper that referenced this pull request Nov 5, 2022
@horizonzy @hangc0276
Fix autoRecover memory leak. (apache#3361)
220cc0b 
(cherry picked from commit da1b29a)
hangc0276 pushed a commit to hangc0276/bookkeeper that referenced this pull request Nov 7, 2022
@horizonzy @hangc0276
Fix autoRecover memory leak. (apache#3361)
2d7c0a5 
(cherry picked from commit da1b29a)
nicoloboschi pushed a commit to datastax/bookkeeper that referenced this pull request Jan 11, 2023
@horizonzy @nicoloboschi
Fix autoRecover memory leak. (apache#3361)
e1744fc 
(cherry picked from commit da1b29a)
(cherry picked from commit 2d7c0a5)
@horizonzy horizonzy mentioned this pull request Jan 19, 2023
Closed
Ghatage pushed a commit to sijie/bookkeeper that referenced this pull request Jul 12, 2024
@horizonzy
Fix autoRecover memory leak. (apache#3361)
be49230
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liuzhuang2017 liuzhuang2017 liuzhuang2017 left review comments

@hangc0276 hangc0276 hangc0276 approved these changes

@dlg99 dlg99 dlg99 approved these changes

@eolivelli eolivelli eolivelli approved these changes

@zymap zymap zymap approved these changes

@merlimat merlimat Awaiting requested review from merlimat

Assignees

@horizonzy horizonzy

Labels
area/autorecovery cherry-picked/branch-4.14 cherry-picked/branch-4.15 release/4.14.6 release/4.15.1 type/bug
Projects
None yet
Milestone
4.16.0
Development

Successfully merging this pull request may close these issues.
6 participants
@horizonzy @zymap @eolivelli @hangc0276 @dlg99 @liuzhuang2017