-
Notifications
You must be signed in to change notification settings - Fork 892
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.
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
[bookie] Fix memory leak when the Bookie is in read only mode #3746
[bookie] Fix memory leak when the Bookie is in read only mode #3746
Conversation
3eb33bb
to
4576422
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
Very detailed investigation and explanation!
if (request instanceof BookieProtocol.ParsedAddRequest) { | ||
((BookieProtocol.ParsedAddRequest) request).release(); | ||
request.recycle(); | ||
requestProcessor.onAddRequestFinish(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the release of ParsedAddRequest be executed in the recycle method? It seems that these two methods are always used together, and there is a forced switch here, which seems a bit awkward.
You can look at org.apache.bookkeeper.proto.BookieProtocol.AddRequest#recycle method
@Override
public void recycle() {
ledgerId = -1;
entryId = -1;
masterKey = null;
ReferenceCountUtil.safeRelease(data);
data = null;
recyclerHandle.recycle(this);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can't, we shouldn't release the data in the recycle.
In the normal case, it will recycle the request, but the data is already free.
See:
bookkeeper/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/WriteEntryProcessor.java
Line 131 in 7b5b6b2
request.recycle(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw it, sorry I misread it earlier. Nice work. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@merlimat @eolivelli @dlg99 @zymap Please help take a look at this PR, thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch
Descriptions of the changes in this PR: Fixes apache#3745 (cherry picked from commit af82d14) (cherry picked from commit 9e0e198)
Descriptions of the changes in this PR: Fixes apache#3745 (cherry picked from commit af82d14) (cherry picked from commit 9e0e198)
Descriptions of the changes in this PR:
Fixes #3745