-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
Slightly expand converage to file consistency check failure #6800
Conversation
Summary: Current DBCompactionTest.ConsistencyFailTest checks DB fails after L0 inconsitency is found. Add slightly more coverage by introducing DBCompactionTest.ConsistencyFailTest2 which checks non-L0 files too. Test Plan: Run the new test.
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.
@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
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 @siying for the test
db/version_builder.cc
Outdated
auto pair = std::make_pair(&f1, &f2); | ||
TEST_SYNC_POINT_CALLBACK("VersionBuilder::CheckConsistency", &pair); | ||
auto pair = std::make_pair(&f1, &f2); | ||
TEST_SYNC_POINT_CALLBACK("VersionBuilder::CheckConsistency1", &pair); |
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.
Nit: How about CheckConsistencyL0
.
db/version_builder.cc
Outdated
@@ -279,6 +279,10 @@ class VersionBuilder::Rep { | |||
NumberToString(f2->fd.GetNumber())); | |||
} | |||
} else { | |||
#ifndef NDEBUG | |||
auto pair = std::make_pair(&f1, &f2); | |||
TEST_SYNC_POINT_CALLBACK("VersionBuilder::CheckConsistency2", &pair); |
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.
How about CheckConsistencyNonL0
// to determine the DB is frozen. | ||
dbfull()->TEST_WaitForCompact(); | ||
ASSERT_NOK(Put("foo", "bar")); | ||
ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->DisableProcessing(); |
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.
Might be safer to clear all callbacks somewhere.
@siying has updated the pull request. Re-import the pull request |
@siying has updated the pull request. Re-import the pull request |
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.
@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@siying has updated the pull request. Re-import the pull request |
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.
@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
This pull request has been merged in c21c459. |
Summary: Current DBCompactionTest.ConsistencyFailTest checks DB fails after L0 inconsitency is found. Add slightly more coverage by introducing DBCompactionTest.ConsistencyFailTest2 which checks non-L0 files too.
Test Plan: Run the new test.