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.

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

[improve][ci] Improve thread leak detection by ignoring more Testcontainers threads #21499

Merged
merged 4 commits into from
Nov 2, 2023

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Nov 1, 2023

Motivation

The thread leak detection added in #21450 reports some Testcontainer threads as leaks. These threads can be safely ignored since they will be terminated eventually.

Modifications

  • detect threads where the Runnable class is under "org.testcontainers" package. Some threads don't have an unique name and the only way to detect them is to check the Runnable's class name.
  • ignore threads where the name starts "testcontainers-wait-"

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@lhotari lhotari added this to the 3.2.0 milestone Nov 1, 2023
@lhotari lhotari self-assigned this Nov 1, 2023
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 1, 2023
@lhotari lhotari force-pushed the lh-ignore-more-testcontainer-threads branch from 9fc8f80 to 8525e87 Compare November 1, 2023 19:26
@lhotari lhotari changed the title [improve][ci] Improve thread leak detection by ignoring more Testcontainer threads [improve][ci] Improve thread leak detection by ignoring more Testcontainers threads Nov 1, 2023
@lhotari lhotari force-pushed the lh-ignore-more-testcontainer-threads branch from 8525e87 to 076fd55 Compare November 1, 2023 21:45
}
return false;
}

private static Runnable extractRunnableTarget(Thread thread) {
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: maybe add a comment on why we have to use reflection here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@codecov-commenter
Copy link

Codecov Report

Merging #21499 (c7546b9) into master (7c6a4b8) will increase coverage by 36.53%.
Report is 3 commits behind head on master.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #21499       +/-   ##
=============================================
+ Coverage     36.76%   73.30%   +36.53%     
- Complexity    12233    32626    +20393     
=============================================
  Files          1713     1890      +177     
  Lines        130816   140423     +9607     
  Branches      14258    15434     +1176     
=============================================
+ Hits          48100   102931    +54831     
+ Misses        76361    29398    -46963     
- Partials       6355     8094     +1739     
Flag Coverage Δ
inttests 24.29% <0.00%> (+0.06%) ⬆️
systests 24.78% <0.00%> (+0.08%) ⬆️
unittests 72.58% <100.00%> (+40.66%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...n/java/org/apache/pulsar/broker/PulsarService.java 82.81% <100.00%> (+13.86%) ⬆️
...rg/apache/pulsar/broker/service/BrokerService.java 80.68% <100.00%> (+25.78%) ⬆️

... and 1453 files with indirect coverage changes

@lhotari lhotari merged commit 8e9ad77 into apache:master Nov 2, 2023
nborisov pushed a commit to nborisov/pulsar that referenced this pull request Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants