Skip to content
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

Attempt to deflake DBTestXactLogIterator.TransactionLogIteratorCorruptedLog #8627

Closed
wants to merge 3 commits into from

Conversation

ltamasi
Copy link
Contributor

@ltamasi ltamasi commented Aug 5, 2021

Summary:
The patch attempts to deflake DBTestXactLogIterator.TransactionLogIteratorCorruptedLog
by disabling file deletions while retrieving the list of WAL files and truncating the first WAL file.
This is to prevent the PurgeObsoleteFiles call triggered by GetSortedWalFiles from
invalidating the result of GetSortedWalFiles. The patch also cleans up the test case a bit
and changes it to using test::TruncateFile instead of calling the truncate syscall directly.

Test Plan:
make check

@facebook-github-bot
Copy link
Contributor

@ltamasi has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

@akankshamahajan15 akankshamahajan15 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 fixing it.

db/db_log_iter_test.cc Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

@ltamasi has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@ltamasi has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@ltamasi has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@ltamasi has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

…tedLog

Summary:
The `truncate` syscall used by this test case sometimes fails on our CI.
The patch changes the test to use `test::TruncateFile` in an attempt to
deflake it.

Test Plan:
`make check`
@facebook-github-bot
Copy link
Contributor

@ltamasi has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@ltamasi has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@ltamasi
Copy link
Contributor Author

ltamasi commented Aug 9, 2021

I believe I've managed to trace the flakiness back to #8591. Based on our discussion with @siying on that PR, I've updated the code to disable file deletions before the GetSortedWalFiles call.

@akankshamahajan15 @riversand963 Since there have been some changes since you've approved, could you take another look at the patch?

@jay-zhuang
Copy link
Contributor

I believe I've managed to trace the flakiness back to #8591. Based on our discussion with @siying on that PR, I've updated the code to disable file deletions before the GetSortedWalFiles call.

@akankshamahajan15 @riversand963 Since there have been some changes since you've approved, could you take another look at the patch?

Is not GetSortedWalFiles() disable the file deletion internally? :

Status s = DisableFileDeletions();

@ltamasi
Copy link
Contributor Author

ltamasi commented Aug 9, 2021

Is not GetSortedWalFiles() disable the file deletion internally? :

Status s = DisableFileDeletions();

Yes but then it re-enables it, which results in an immediate purge, potentially invalidating the result of the call before the caller gets to use it. Disabling/enabling also at the call site prevents this from happening, since disable_delete_obsolete_files_ is really a counter; i.e. you have to re-enable purge as many times as you have disabled it.

@facebook-github-bot
Copy link
Contributor

@ltamasi merged this pull request in f63331e.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants