Skip to content

Conversation

@rickyma
Copy link
Contributor

@rickyma rickyma commented May 23, 2024

What changes were proposed in this pull request?

Fix various flaky tests:

  1. After using ApplicationManager, release resources at once, which will let ScheduledExecutorService work background to cause unexpected behaviors.
  2. Release DynamicClientConfService after using it.
  3. Release SimpleClusterManager everytime after using it. Since the execution order of JUnit test methods is random, unless we specifically set the execution order. So it is better not to initialize the SimpleClusterManager object globally.
  4. Wait for a while to ensure the port is released in UniffleJavaProcess.
  5. Always call stopServer after new CoordinatorServer() to shut down multiple ExecutorService from running in background, which could cause unexpected behaviors.

Why are the changes needed?

For #1675.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Unnecessary.

@rickyma rickyma force-pushed the issue-pr-1730-followup branch from 0f17b4e to 8fc9f02 Compare May 23, 2024 17:27
@github-actions
Copy link

Test Results

 2 419 files  ±0   2 419 suites  ±0   5h 1m 14s ⏱️ + 3m 21s
   933 tests ±0     932 ✅ ±0   1 💤 ±0  0 ❌ ±0 
10 819 runs  ±0  10 805 ✅ ±0  14 💤 ±0  0 ❌ ±0 

Results for commit 8fc9f02. ± Comparison against base commit a0e88da.

Copy link
Member

@zuston zuston left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for your effort for the better + stable tests.

Although it looks a bit wire for every resource manager to be wrapped in the try . (Java lacks the drop semantic.)

@zuston zuston changed the title [#1675][FOLLOWUP] fix(test): Fix flaky tests which could cause unexpected behaviors [#1675][FOLLOWUP] fix(test): Explicitly close resources to avoid unexcepted behaviors May 28, 2024
@zuston zuston merged commit 417674d into apache:master May 28, 2024
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.

2 participants