[Tests] Fix thread leak in MLTransactionMetadataStore #14524
Merged
Conversation
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
lhotari
requested review from
codelipenghui,
congbobo184,
eolivelli,
gaoran10,
merlimat and
michaeljmarshall
4 tasks
Member
Author
|
@lordcheng10 Could this help in reducing memory consumption of branch-2.10 (v2.10.0-candidate-2) & master branch version? |
liangyepianzhou
approved these changes
Mar 2, 2022
- MLTransactionMetadataStore.internalPinnedExecutor wasn't closed when MLTransactionMetadataStore.closeAsync was called - problem was introduced by apache#14238 changes - this issue causes tests to fail with OOME. Most likely this also impacts production code.
lhotari
force-pushed
the
lh-fix-OOME-in-tests
branch
from
5821821 to
ceb3ff1
Compare
eolivelli
reviewed
Mar 2, 2022
Contributor
eolivelli
left a comment
eolivelli
left a comment
There was a problem hiding this comment.
I left one suggestion PTAL
...src/main/java/org/apache/pulsar/transaction/coordinator/impl/MLTransactionMetadataStore.java
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
Outdated
Show resolved
Hide resolved
Member
Author
|
btw. The thread leak is visible also in test output. #10195 added https://github.com/apache/pulsar/blob/master/buildtools/src/main/java/org/apache/pulsar/tests/ThreadLeakDetectorListener.java which logs threads that are active when the test ends. For example, when running this can be seen in the console: |
Member
Author
|
@codelipenghui Is the thread leak a blocker for 2.10 release? |
lhotari
added
the
release/blocker
codelipenghui
approved these changes
Mar 2, 2022
Contributor
|
@lhotari Yes, this should be a blocker for 2.10.0 release, will cherry-pick this PR and start a new VOTE for 2.10.0 |
codelipenghui
pushed a commit
that referenced
this pull request
Mar 2, 2022
- MLTransactionMetadataStore.internalPinnedExecutor wasn't closed when MLTransactionMetadataStore.closeAsync was called - problem was introduced by #14238 changes - this issue causes tests to fail with OOME. Most likely this also impacts production code. * Close TransactionMetadataStoreService after the broker service has been closed (cherry picked from commit 0ddec86)
michaeljmarshall
approved these changes
Mar 2, 2022
gaoran10
pushed a commit
that referenced
this pull request
Mar 11, 2022
- MLTransactionMetadataStore.internalPinnedExecutor wasn't closed when MLTransactionMetadataStore.closeAsync was called - problem was introduced by #14238 changes - this issue causes tests to fail with OOME. Most likely this also impacts production code. * Close TransactionMetadataStoreService after the broker service has been closed (cherry picked from commit 0ddec86)
gaoran10
added
cherry-picked/branch-2.9
Nicklee007
pushed a commit
to Nicklee007/pulsar
that referenced
this pull request
Apr 20, 2022
- MLTransactionMetadataStore.internalPinnedExecutor wasn't closed when MLTransactionMetadataStore.closeAsync was called - problem was introduced by apache#14238 changes - this issue causes tests to fail with OOME. Most likely this also impacts production code. * Close TransactionMetadataStoreService after the broker service has been closed
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 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
MLTransactionMetadataStore.internalPinnedExecutor wasn't closed
when MLTransactionMetadataStore.closeAsync was called
this issue causes tests to fail with OOME. This might also impact
production code in some way.
example OOME in tests
Modifications
MoreExecutors.shutdownAndAwaitTerminationto handle ExecutorService shutdown since it handles shutdown in a proper way gracefully although in this case it seems that it would be fine to terminate forcefully by calling shutdownNow