-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[ML] Release OpAddEntry.data when entry is copied and discarded #10773
[ML] Release OpAddEntry.data when entry is copied and discarded #10773
Conversation
@@ -1504,6 +1504,8 @@ public synchronized void updateLedgersIdsComplete(Stat stat) { | |||
if (existsOp.ledger != null) { | |||
existsOp.close(); | |||
existsOp = OpAddEntry.create(existsOp.ml, existsOp.data, existsOp.getNumberOfMessages(), existsOp.callback, existsOp.ctx); | |||
// release the extra retain | |||
ReferenceCountUtil.release(existsOp.data); |
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.
Just one question, doesn't existsOp.close
release the reference? Or can you clarify (potentially comment in the code) why do we need to release extra retain here?
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.
Just one question, doesn't existsOp.close release the reference?
No it doesn't.
why do we need to release extra retain here?
OpAddEntry.create will call .retain , that's the extra retain. I already tried to clarify that in the comment in the code. Does it need more clarification?
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, just add more context here. We can safe to release the buffer here because we closed the old OpAddEntry, if the OpAddEntry in the closed state, even if the addComplete
been called, will not use the buffer again.
if (!STATE_UPDATER.compareAndSet(OpAddEntry.this, State.INITIATED, State.COMPLETED)) {
log.warn("[{}] The add op is terminal legacy callback for entry {}-{} adding.", ml.getName(), lh.getId(), entryId);
OpAddEntry.this.recycle();
return;
}
Linking to #6054 for tracking |
@@ -1504,6 +1504,8 @@ public synchronized void updateLedgersIdsComplete(Stat stat) { | |||
if (existsOp.ledger != null) { | |||
existsOp.close(); | |||
existsOp = OpAddEntry.create(existsOp.ml, existsOp.data, existsOp.getNumberOfMessages(), existsOp.callback, existsOp.ctx); | |||
// release the extra retain | |||
ReferenceCountUtil.release(existsOp.data); |
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, just add more context here. We can safe to release the buffer here because we closed the old OpAddEntry, if the OpAddEntry in the closed state, even if the addComplete
been called, will not use the buffer again.
if (!STATE_UPDATER.compareAndSet(OpAddEntry.this, State.INITIATED, State.COMPLETED)) {
log.warn("[{}] The add op is terminal legacy callback for entry {}-{} adding.", ml.getName(), lh.getId(), entryId);
OpAddEntry.this.recycle();
return;
}
) Fixes apache#10738 ### Motivation There's a ByteBuf leak that happens in ledger rollover. The repro case in apache#10738 reproduces some Netty ByteBuf leaks that are detected by the Netty Leak detector. ### Modifications Release `OpAddEntry.data` when the original entry is copied and discarded ### Additional Context This PR was split out of PR apache#10755 which was closed without merging.
Fixes #10738 There's a ByteBuf leak that happens in ledger rollover. The repro case in #10738 reproduces some Netty ByteBuf leaks that are detected by the Netty Leak detector. Release `OpAddEntry.data` when the original entry is copied and discarded This PR was split out of PR #10755 which was closed without merging.
Fixes #10738 There's a ByteBuf leak that happens in ledger rollover. The repro case in #10738 reproduces some Netty ByteBuf leaks that are detected by the Netty Leak detector. Release `OpAddEntry.data` when the original entry is copied and discarded This PR was split out of PR #10755 which was closed without merging.
) Fixes apache#10738 There's a ByteBuf leak that happens in ledger rollover. The repro case in apache#10738 reproduces some Netty ByteBuf leaks that are detected by the Netty Leak detector. Release `OpAddEntry.data` when the original entry is copied and discarded This PR was split out of PR apache#10755 which was closed without merging. (cherry picked from commit 121305f)
) Fixes apache#10738 ### Motivation There's a ByteBuf leak that happens in ledger rollover. The repro case in apache#10738 reproduces some Netty ByteBuf leaks that are detected by the Netty Leak detector. ### Modifications Release `OpAddEntry.data` when the original entry is copied and discarded ### Additional Context This PR was split out of PR apache#10755 which was closed without merging.
) Fixes apache#10738 ### Motivation There's a ByteBuf leak that happens in ledger rollover. The repro case in apache#10738 reproduces some Netty ByteBuf leaks that are detected by the Netty Leak detector. ### Modifications Release `OpAddEntry.data` when the original entry is copied and discarded ### Additional Context This PR was split out of PR apache#10755 which was closed without merging.
Fixes #10738
Motivation
There's a ByteBuf leak that happens in ledger rollover. The repro case in #10738 reproduces some Netty ByteBuf leaks that are detected by the Netty Leak detector.
Modifications
Release
OpAddEntry.data
when the original entry is copied and discardedAdditional Context
This PR was split out of PR #10755 which was closed without merging.