-
Notifications
You must be signed in to change notification settings - Fork 226
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
Implement thread leakage check, fixing major thread leakage test and some flaky test #1227
Conversation
helix-core/src/test/java/org/apache/helix/common/ZkTestBase.java
Outdated
Show resolved
Hide resolved
cc2fcd1
to
df4056c
Compare
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.
Please clean up the code. You have too many system.out.println. It could cause the test have more messed up info in test result.
helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java
Outdated
Show resolved
Hide resolved
helix-core/src/main/java/org/apache/helix/task/TaskStateModelFactory.java
Outdated
Show resolved
Hide resolved
helix-core/src/test/java/org/apache/helix/TestListenerCallback.java
Outdated
Show resolved
Hide resolved
helix-core/src/test/java/org/apache/helix/common/ZkTestBase.java
Outdated
Show resolved
Hide resolved
helix-core/src/test/java/org/apache/helix/common/ZkTestBase.java
Outdated
Show resolved
Hide resolved
helix-core/src/test/java/org/apache/helix/ThreadLeakageChecker.java
Outdated
Show resolved
Hide resolved
29f7fed
to
c80f573
Compare
@kaisun2000 Let's don't keep adding more changes to the same PR. I think you have put 2 PRs into one change. It should be at least, 1. thread leakage check. 2. fixing the tests (I would prefer fixing them separately, but if the fixes are similar then it's OK). |
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.
@kaisun2000 I strongly recommend splitting the logic changes from the pure testing fixes.
I mainly reviewed the logic change part. The test changes are too long to review. Could you please re-org the PR?
helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java
Outdated
Show resolved
Hide resolved
helix-core/src/main/java/org/apache/helix/tools/ClusterStateVerifier.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/apache/helix/tools/ClusterVerifiers/BestPossibleExternalViewVerifier.java
Outdated
Show resolved
Hide resolved
helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
Outdated
Show resolved
Hide resolved
Let us sync about it offline. There are conflict of different requirements. 1/ If our goal is to have all the test pass in github, we have to run it consistently till it converge to no failing test with all the changes in. Otherwise, how can we verify? |
a5759c6
to
74add13
Compare
.../src/main/java/org/apache/helix/tools/ClusterVerifiers/BestPossibleExternalViewVerifier.java
Outdated
Show resolved
Hide resolved
helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
Outdated
Show resolved
Hide resolved
helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
Outdated
Show resolved
Hide resolved
101d146
to
7f4965f
Compare
a38e3e2
to
7664dff
Compare
shutdownNow() not public.
…ementAfterDelayTime from merge. Added it back.
separate pr now.
dfcd336
to
f3cbb1e
Compare
Close due to inactive. |
resolve #1226, #1239, #1254, #1247, #1246, #1245, #1244, #1241, #1240 etc a bunch of unstable test
Description
There are some unit test not stable for various reasons. This PR addressed them.
Tests
The following tests are written for this issue:
The following is the result of the "mvn test" command on the appropriate module:
git hub run.
(Link the GitHub wiki you added)
Commits
Code Quality
(helix-style-intellij.xml if IntelliJ IDE is used)