-
Notifications
You must be signed in to change notification settings - Fork 13.8k
[FLINK-35801] Fix SnapshotFileMergingCompatibilityITCase, wait for file deletion before quit #25066
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
Conversation
fredia
left a comment
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.
@Zakelly Thanks for the quick fix, overall LGTM.
| Thread.sleep(500L); | ||
| waited += 500L; | ||
| // Or timeout | ||
| assertThat(waited).isLessThan(DELETE_TIMEOUT_MILLS); |
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.
If the timeout is reached and the files are not deleted, will this check failed?
If restoring from file-merging disabled to file-merging disabled, is it okay to skip this check?
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.
If the timeout is reached and the files are not deleted, will this check failed?
Yes.
If restoring from
file-merging disabledtofile-merging disabled, is it okay to skip this check?
In this case, we are not testing the file-merging, but we could check the file existence by the way.
|
I don't think the CI failure is related with file-merging. It happens when file-merging is disabled. I'm investigating the potential file leak. Update: Well there is an issue with the test code. |
…le deletion before quit
|
I have run the test with the last commit for 200 times. No failure. |
fredia
left a comment
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.
Thanks for investigating and updating, let's wait CI green.
…le deletion before quit (apache#25066)
What is the purpose of the change
This PR fixes the unstable test
SnapshotFileMergingCompatibilityITCase. There is a problem with the test code itself. The test verifies the cp files are cleaned as expected, which depends on an async cleaner (the job io thread). While the mini-cluster quit, the async threads may not have finished their jobs before termination.It is better to try multiple rounds AZP to verify this fix.
Brief change log
Only
SnapshotFileMergingCompatibilityITCase. Make it wait for file deletion before cluster termination.Verifying this change
Does this pull request potentially affect one of the following parts:
@Public(Evolving): noDocumentation