Skip to content

Comments

thread leakage checker and memory usage reporter #1226#1452

Merged
jiajunwang merged 6 commits intoapache:masterfrom
kaisun2000:threadleak_mem_check_#1226
Oct 9, 2020
Merged

thread leakage checker and memory usage reporter #1226#1452
jiajunwang merged 6 commits intoapache:masterfrom
kaisun2000:threadleak_mem_check_#1226

Conversation

@kaisun2000
Copy link
Contributor

@kaisun2000 kaisun2000 commented Oct 8, 2020

Issues

  • My PR addresses the following Helix issues and references them in the PR description:

fix #1226

Description

  • Here are some details about my PR, including screenshots of any UI changes:

Add thread leakage checker and memory usage reporter. The two
utility would be invoke before and after test classes. The would
help to detect/monitor resource/memory usage of the unit test.

Tests

  • The following tests are written for this issue:

(List the names of added unit/integration tests)

  • The following is the result of the "mvn test" command on the appropriate module:

2020-10-08T07:55:24.0931119Z
2020-10-08T07:55:24.4922133Z [ERROR] Failures:
2020-10-08T07:55:24.4923510Z [ERROR] TestDisableCustomCodeRunner.test:236 expected: but was:
2020-10-08T07:55:24.4925729Z [ERROR] TestClusterStatusMonitorLifecycle.testClusterStatusMonitorLifecycle:290 expected: but was:
2020-10-08T07:55:24.4928050Z [ERROR] Tests run: 1213, Failures: 2, Errors: 0, Skipped: 0
re-run would work.

Documentation (Optional)

  • In case of new functionality, my PR adds documentation in the following wiki page:

(Link the GitHub wiki you added)

Commits

  • My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Code Quality

  • My diff has been formatted using helix-style.xml
    (helix-style-intellij.xml if IntelliJ IDE is used)

Add thread leakage checker and memory usage reporter. The two
utility would be invoke before and after test classes. The would
help to detect/monitor resource/memory usage of the unit test.
@lei-xia
Copy link
Member

lei-xia commented Oct 8, 2020

Are we targeting to output the memory usage after each test class? Will this PR make it happen?

@kaisun2000
Copy link
Contributor Author

Are we targeting to output the memory usage after each test class? Will this PR make it happen?

Currently, all the ones using ZkTestBase. Since the majority use ZkTestBase, we have a good idea of the memory usage over time.

@kaisun2000
Copy link
Contributor Author

The PR is approved, please help to merge in.

Add thread leakage checker and memory usage reporter. The two
utility would be invoke before and after test classes. The would
help to detect/monitor resource/memory usage of the unit test.

Copy link
Contributor

@jiajunwang jiajunwang left a comment

Choose a reason for hiding this comment

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

Still, 2 problems that I also mentioned in the other test improvement PRs.

  1. What is the guideline for using a system print vs. log? Without confirming this point, I cannot review it effectively. I prefer log, BTW.
  2. Why not throw an exception on leakage? If just print error, then we can just let the tool runs every night. There is no need to do it with every "mvn test".
    For example, if people ignore the error, then we might figure it out weeks later. Then this check will be very less helpful.

@kaisun2000
Copy link
Contributor Author

Still, 2 problems that I also mentioned in the other test improvement PRs.

  1. What is the guideline for using a system print vs. log? Without confirming this point, I cannot review it effectively. I prefer log, BTW.
  2. Why not throw an exception on leakage? If just print error, then we can just let the tool runs every night. There is no need to do it with every "mvn test".
    For example, if people ignore the error, then we might figure it out weeks later. Then this check will be very less helpful.

If we throw exception, current test won't work. I reduced the threads leakage from around 3000 to several hundreds at this point. Getting it to 0 is still some way to go.

@kaisun2000
Copy link
Contributor Author

Sync-ed with JJ offline. Once the thread leakage reduced to 0. We will enable the throw exception (failing test). Before that, we use system.out to log it out for people to examine without enable info level logging

@kaisun2000
Copy link
Contributor Author

The PR is approved. Please help to merge to trunk

Add thread leakage checker and memory usage reporter. The two
utility would be invoke before and after test classes. The would
help to detect/monitor resource/memory usage of the unit test.

@jiajunwang jiajunwang merged commit 00d4fd8 into apache:master Oct 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Checking Resource Leak at end of a test class

5 participants