-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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 MultiGet dropping memtable kv checksum corruption #12842
Conversation
@cbi42 has imported this pull request. If you are a Meta 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.
History needs to be updated
db/memtable.cc
Outdated
@@ -1310,7 +1314,7 @@ bool MemTable::Get(const LookupKey& key, std::string* value, | |||
|
|||
// No change to value, since we have not yet found a Put/Delete | |||
// Propagate corruption error | |||
if (!found_final_value && merge_in_progress && !s->IsCorruption()) { | |||
if (!found_final_value && merge_in_progress && s->ok()) { |
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.
Should we assert(s,ok())
? If non-ok status, then found_final_value
should always be true right?
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.
It can also be merge in process, added assert on this.
720d30c
to
9f050b9
Compare
@cbi42 has updated the pull request. You must reimport the pull request before landing. |
Added, thanks for the review. |
@cbi42 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Corruption status returned by
GetFromTable()
could be overwritten here:rocksdb/db/version_set.cc
Line 2614 in b6c3495
This PR fixes this issue by setting
*(s->found_final_value) = true;
in SaveValue. Also makes the handling of the return value ofGetFromTable()
more robust and added asserts in a couple places.Test plan: Updated an existing unit test to cover MultiGet. It fails the assertion here before this PR:
rocksdb/db/version_set.cc
Line 2601 in b6c3495