-
Notifications
You must be signed in to change notification settings - Fork 134
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
[ISSUE-514] Fix flaky test: ShuffleServerGrpcTest#clearResourceTest #516
Conversation
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.
LGTM, thanks @xianjingfeng for the fix.
@@ -160,7 +160,8 @@ public void clearResourceTest() throws Exception { | |||
|
|||
// clearResourceTest1 will be removed because of rss.server.app.expired.withoutHeartbeat | |||
t.interrupt(); | |||
Awaitility.await().timeout(20, TimeUnit.SECONDS).until(() -> shuffleServers.get(0).getShuffleTaskManager().getAppIds().size() == 0); | |||
Awaitility.await().timeout(20, TimeUnit.SECONDS).until( | |||
() -> shuffleServers.get(0).getShuffleTaskManager().getAppIds().size() == 0); | |||
assertEquals(0, shuffleServers.get(0).getShuffleTaskManager().getAppIds().size()); |
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.
This line is not needed now.
However, it's OK to keep it.
Codecov Report
@@ Coverage Diff @@
## master #516 +/- ##
============================================
+ Coverage 59.73% 59.75% +0.01%
- Complexity 1764 1765 +1
============================================
Files 205 205
Lines 11527 11527
Branches 1033 1033
============================================
+ Hits 6886 6888 +2
+ Misses 4234 4233 -1
+ Partials 407 406 -1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
What changes were proposed in this pull request?
Fix flaky test: ShuffleServerGrpcTest#clearResourceTest. Fix #514
Why are the changes needed?
It is flaky test.
In this UT,
rss.server.preAllocation.expired=5000
, soShuffleTaskManager#checkResourceStatus
will be invoked for every 2.5 seconds and expired app may will not be removed until 7.5 seconds after stop heartbeat. So sleep 8 seconds may not be reliable.Does this PR introduce any user-facing change?
No
How was this patch tested?
No need