dolysis/rocksdb
3
0
Fork 0
mirror of https://github.com/facebook/rocksdb.git synced 2024-11-27 02:44:18 +00:00
Code Issues Packages Projects Releases Wiki Activity

Fix a group commit bug in LogAndApply

Browse source 
Summary:
EncodeTo(&record) does not overwrite, it appends to it.

This means that group commit log and apply will look something like:
record1
record1record2
record1record2record3

I'm surprised this didn't show up in production, but I think the reason is that MANIFEST group commit almost never happens.

This bug turned up in column family work, where opening a database failed with "adding a same column family twice".

Test Plan: Tested the change in column family branch and observed that the problem is gone (with db_stress)

Reviewers: dhruba, haobo

Reviewed By: dhruba

CC: leveldb

Differential Revision: https://reviews.facebook.net/D16461
This commit is contained in:
Igor Canadi 2014-03-03 17:10:43 -08:00
parent 97eddef235
commit 5142b37000
1 changed files with 13 additions and 4 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

15
db/version_set.cc
View file

@ -1568,9 +1568,9 @@ Status VersionSet::LogAndApply(VersionEdit* edit, port::Mutex* mu,
// Write new record to MANIFEST log
if (s.ok()) {
for (auto& e : batch_edits) {
std::string record;
for (unsigned int i = 0; i < batch_edits.size(); i++) {
batch_edits[i]->EncodeTo(&record);
e->EncodeTo(&record);
s = descriptor_log_->AddRecord(record);
if (!s.ok()) {
break;
@ -1589,7 +1589,16 @@ Status VersionSet::LogAndApply(VersionEdit* edit, port::Mutex* mu,
}
if (!s.ok()) {
Log(options_->info_log, "MANIFEST write: %s\n", s.ToString().c_str());
if (ManifestContains(record)) {
bool all_records_in = true;
for (auto& e : batch_edits) {
std::string record;
e->EncodeTo(&record);
if (!ManifestContains(record)) {
all_records_in = false;
break;
}
}
if (all_records_in) {
Log(options_->info_log,
"MANIFEST contains log record despite error; advancing to new "
"version to prevent mismatch between in-memory and logged state"