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 ByteBuf memory leak problem when setExplicitLac #3577

Merged
merged 4 commits into from
Nov 1, 2022
Merged

Fix ByteBuf memory leak problem when setExplicitLac #3577

merged 4 commits into from
Nov 1, 2022

Conversation

wenbingshen
Copy link
Member

Motivation

org.apache.bookkeeper.bookie.BookieImpl#setExplicitLac did not release the reference to the entry at the end.

@wenbingshen
Copy link
Member Author

ping @dlg99 @eolivelli @hangc0276 @zymap PTAL. Thanks.

hangc0276
hangc0276 requested changes Oct 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.

@wenbingshen Would you please add a test to cover it?

@wenbingshen
Copy link
Member Author

@wenbingshen Would you please add a test to cover it?

@hangc0276 Okay. I will add a test later. Thanks.

@wenbingshen
Copy link
Member Author

@hangc0276 I have added an unit test. PTAL. Thanks.
When I apply the changes from this PR, the tests pass:
image

When I roll back the changes from this PR, the test fails:
image

The effect is as expected.

@wenbingshen wenbingshen changed the title Fix set explicitLac no released entry Fix ByteBuf memory leak problem when setExplicitLac Oct 27, 2022
eolivelli
eolivelli approved these changes Oct 27, 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.

Great work

hangc0276
hangc0276 approved these changes Oct 28, 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.

Nice work!

@hangc0276 hangc0276 added this to the 4.16.0 milestone Oct 28, 2022
StevenLuMT
StevenLuMT requested changes Oct 28, 2022
View reviewed changes
Copy link
Contributor

@StevenLuMT StevenLuMT left a comment •
edited

Choose a reason for hiding this comment

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

I think we can update the other old code to replace entry.release() to ReferenceCountUtil.safeRelease(entry)

or else I pick up a new pr to do this ? have a look this @hangc0276 @eolivelli @dlg99 @shoothzj

@zymap
Copy link
Member

zymap commented Oct 31, 2022

@StevenLuMT I think we could use another PR to resolve that.

@wenbingshen
Copy link
Member Author

@StevenLuMT I think we could use another PR to resolve that.

I agree.

@wenbingshen
Copy link
Member Author

@StevenLuMT PTAL. Thanks.

StevenLuMT
StevenLuMT approved these changes Nov 1, 2022
View reviewed changes
Copy link
Contributor

@StevenLuMT StevenLuMT left a comment

Choose a reason for hiding this comment

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

LGTM

@StevenLuMT StevenLuMT merged commit faaf041 into apache:master Nov 1, 2022
@wenbingshen wenbingshen deleted the FixSetExplicitLacMemLeak branch November 2, 2022 02:37
zymap pushed a commit that referenced this pull request Nov 3, 2022
@wenbingshen @zymap
Fix ByteBuf memory leak problem when setExplicitLac (#3577)
4e8f375 
* fix set explicitLac no released entry

(cherry picked from commit faaf041)
@hangc0276
Copy link
Contributor

@wenbingshen It's hard to cherry-pick this pr to branch-4.14, would you please push a separate Pr to branch-4.14? thanks

@wenbingshen
Copy link
Member Author

wenbingshen commented Nov 7, 2022 •
edited

@wenbingshen It's hard to cherry-pick this pr to branch-4.14, would you please push a separate Pr to branch-4.14? thanks

@hangc0276 Okay. I will push a pr to 4.14 today.

@wenbingshen wenbingshen mentioned this pull request Nov 7, 2022
Merged
yaalsn pushed a commit to yaalsn/bookkeeper that referenced this pull request Jan 30, 2023
@wenbingshen @yaalsn
Fix ByteBuf memory leak problem when setExplicitLac (apache#3577)
6d8784c 
* fix set explicitLac no released entry
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hangc0276 hangc0276 hangc0276 approved these changes

@eolivelli eolivelli eolivelli approved these changes

@shoothzj shoothzj shoothzj approved these changes

@StevenLuMT StevenLuMT StevenLuMT approved these changes

@nicoloboschi nicoloboschi Awaiting requested review from nicoloboschi

@dlg99 dlg99 Awaiting requested review from dlg99

@merlimat merlimat Awaiting requested review from merlimat

@zymap zymap Awaiting requested review from zymap

Assignees

@wenbingshen wenbingshen

Labels
cherry-picked/branch-4.15 release/4.15.3 type/bug
Projects
None yet
Milestone
4.16.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@wenbingshen @zymap @hangc0276 @eolivelli @shoothzj @StevenLuMT