[fix][broker] Close transactionBuffer after MessageDeduplication#checkStatus failed#19157
Merged
codelipenghui merged 4 commits intoapache:masterfrom Jan 17, 2023
Merged
Conversation
Codecov Report
@@ Coverage Diff @@
## master #19157 +/- ##
============================================
+ Coverage 46.24% 47.30% +1.05%
- Complexity 10145 10718 +573
============================================
Files 687 713 +26
Lines 67483 69731 +2248
Branches 7252 7497 +245
============================================
+ Hits 31208 32985 +1777
- Misses 32693 33030 +337
- Partials 3582 3716 +134
Flags with carried forward coverage won't be shown. Click here to find out more.
|
congbobo184
reviewed
Jan 9, 2023
Contributor
congbobo184
left a comment
congbobo184
left a comment
There was a problem hiding this comment.
better to add a test for it
yaalsn
reviewed
Jan 10, 2023
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
Show resolved
Hide resolved
...er/src/test/java/org/apache/pulsar/broker/transaction/buffer/TopicTransactionBufferTest.java
Show resolved
Hide resolved
codelipenghui
approved these changes
Jan 17, 2023
Contributor
|
@tjiuming Could you please help create a PR to cherry-pick the fix to branch-2.11, branch-2.10 and branch-2.9? |
codelipenghui
added
release/2.11.1
release/2.9.5
release/2.10.4
type/bug
tjiuming
added a commit
to tjiuming/pulsar
that referenced
this pull request
Jan 19, 2023
…kStatus failed (apache#19157) (cherry picked from commit d25cf8e)
4 tasks
tjiuming
added a commit
to tjiuming/pulsar
that referenced
this pull request
Jan 19, 2023
…kStatus failed (apache#19157) (cherry picked from commit d25cf8e)
4 tasks
tjiuming
added a commit
to tjiuming/pulsar
that referenced
this pull request
Jan 19, 2023
…kStatus failed (apache#19157) (cherry picked from commit d25cf8e)
4 tasks
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Currently, we didn't close the
TransactionBufferafterMessageDeduplication#checkStatusfailed. see: https://github.com/apache/pulsar/blob/master/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java#L1600it will lead to memory leak.
In
TopicTransactionBuffer, it will create asnapshotWriteinSingleSnapshotAbortedTxnProcessorImpl, and the write will held bySystemTopicClientBase#writers.If we don't close the
TransactionBufferafterMessageDeduplication#checkStatusfailed, the number ofTransactionBufferSnapshotWriterheld bySystemTopicClientBase#writerswill keep increasing, and may lead to OOM in the finally.In #15015 , it reuses the
TransactionBufferSnapshotWriter, which means that the topics of a same namespace will use one singleTransactionBufferSnapshotWriterinstance, it greatly reduces the amount of memory leaks, but if we don't close theTransactionBuffer, there are still memory leak risks.Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: tjiuming#17