Skip to content

Conversation

maobaolong
Copy link
Member

What changes were proposed in this pull request?

Fix flaky test for CoordinatorReconfigureNodeMaxTest#testReconfigureNodeMax

Why are the changes needed?

Make CI stable.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing UTs.

Copy link

github-actions bot commented Jan 5, 2025

Test Results

 2 966 files  ±0   2 966 suites  ±0   6h 35m 54s ⏱️ + 8m 36s
 1 100 tests ±0   1 098 ✅ ±0   2 💤 ±0  0 ❌ ±0 
13 774 runs  ±0  13 744 ✅ ±0  30 💤 ±0  0 ❌ ±0 

Results for commit 39ee790. ± Comparison against base commit 3b0521a.

♻️ This comment has been updated with latest results.

@maobaolong maobaolong closed this Jan 5, 2025
@maobaolong maobaolong reopened this Jan 5, 2025
jerqi
jerqi previously approved these changes Jan 7, 2025
assertEquals(5, info.getServerToPartitionRanges().keySet().size());

// case3: recover its value to 10
Thread.sleep(1000L);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should Awaitility.await.

Copy link
Member Author

@maobaolong maobaolong Jan 8, 2025

Choose a reason for hiding this comment

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

@jerqi I added a new comment above this line. This sleep to make sure the last modification time of the tempConfFilePath file changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

You wait for a modification timestamp instead sleep.

File tempConfFile = new File(tempConfFilePath);
long currentTime = System.currentTimeMillis();
if (currentTime - tempConfFile.lastModified() < 1000) {
Thread.sleep(1000 - (currentTime - tempConfFile.lastModified()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Could use Awaitility.await() instead of sleep?

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