-
Notifications
You must be signed in to change notification settings - Fork 6.1k
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
Fix a unit test hole for recovering UDTs with WAL files #11577
Conversation
1292fcf
to
b8b24b5
Compare
@jowlyzhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
b8b24b5
to
27ceb99
Compare
@jowlyzhang has updated the pull request. You must reimport the pull request before landing. |
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!
Don't forget to re-import :) |
27ceb99
to
535f0a0
Compare
@jowlyzhang has updated the pull request. You must reimport the pull request before landing. |
Thank you for your help reviewing this PR and the reminder! |
@jowlyzhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@jowlyzhang merged this pull request in baf37a0. |
Thanks @pdillinger for pointing out this test hole. The test
DBWALTestWithTimestamp.Recover
that is intended to test recovery from WAL including user-defined timestamps doesn't achieve its promised coverage. Specifically, after #11557, timestamps will be removed during flush, and RocksDB by default flush memtables during recovery withavoid_flush_during_recovery
defaults to false. This test didn't fail even if all the timestamps are quickly lost due to the default flush behavior.This PR renamed test
Recover
toRecoverAndNoFlush
, and updated it to verify timestamps are successfully recovered from WAL with some time-travel reads.avoid_flush_during_recovery
is set to true to help do this verification.On the other hand, for test
DBWALTestWithTimestamp.RecoverAndFlush
, since flush on reopen is DB's default behavior. Setting the flagsmax_write_buffer
andarena_block_size
are not really the factors that enforces the flush, so these flags are removed.Test Plan:
./db_wal_test