Skip to content

Commit

Permalink
Fix flaky, dubious LdbCmdTest::*DumpFileChecksum* (#8898)
Browse files Browse the repository at this point in the history 
Summary:
These tests would frequently fail to find SST files due to race
condition in running ldb (read-only) on an open DB which might do automatic
compaction. But only sometimes would that failure translate into test
failure because the implementation of ldb file_checksum_dump would
swallow many errors. Now,

* DB closed while running ldb to avoid unnecessary race condition
* Detect and report/propagate more failures in `ldb file_checksum_dump`
* Use --hex so that random binary data is not printed to console

Pull Request resolved: #8898

Test Plan: ./ldb_cmd_test --gtest_filter=*Checksum* --gtest_repeat=100

Reviewed By: zhichao-cao

Differential Revision: D30848738

Pulled By: pdillinger

fbshipit-source-id: 20290b517eeceba99bb538bb5a17088f7e878405
  • Loading branch information
@pdillinger @facebook-github-bot
pdillinger authored and facebook-github-bot committed Sep 14, 2021
1 parent 7bef598 commit a5566d5
Show file tree
Hide file tree
Showing 2 changed files with 65 additions and 28 deletions.
26 changes: 14 additions & 12 deletions tools/ldb_cmd.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1206,9 +1206,9 @@ void ManifestDumpCommand::DoCommand() {
// ----------------------------------------------------------------------------
namespace {

void GetLiveFilesChecksumInfoFromVersionSet(Options options,
const std::string& db_path,
FileChecksumList* checksum_list) {
Status GetLiveFilesChecksumInfoFromVersionSet(Options options,
const std::string& db_path,
FileChecksumList* checksum_list) {
EnvOptions sopt;
Status s;
std::string dbname(db_path);
Expand Down Expand Up @@ -1238,9 +1238,7 @@ void GetLiveFilesChecksumInfoFromVersionSet(Options options,
if (s.ok()) {
s = versions.GetLiveFilesChecksumInfo(checksum_list);
}
if (!s.ok()) {
fprintf(stderr, "Error Status: %s", s.ToString().c_str());
}
return s;
}

} // namespace
Expand Down Expand Up @@ -1279,14 +1277,14 @@ void FileChecksumDumpCommand::DoCommand() {
// ......

std::unique_ptr<FileChecksumList> checksum_list(NewFileChecksumList());
GetLiveFilesChecksumInfoFromVersionSet(options_, db_path_,
checksum_list.get());
if (checksum_list != nullptr) {
Status s = GetLiveFilesChecksumInfoFromVersionSet(options_, db_path_,
checksum_list.get());
if (s.ok() && checksum_list != nullptr) {
std::vector<uint64_t> file_numbers;
std::vector<std::string> checksums;
std::vector<std::string> checksum_func_names;
Status s = checksum_list->GetAllFileChecksums(&file_numbers, &checksums,
&checksum_func_names);
s = checksum_list->GetAllFileChecksums(&file_numbers, &checksums,
&checksum_func_names);
if (s.ok()) {
for (size_t i = 0; i < file_numbers.size(); i++) {
assert(i < file_numbers.size());
Expand All @@ -1301,8 +1299,12 @@ void FileChecksumDumpCommand::DoCommand() {
fprintf(stdout, "%" PRId64 ", %s, %s\n", file_numbers[i],
checksum_func_names[i].c_str(), checksum.c_str());
}
fprintf(stdout, "Print SST file checksum information finished \n");
}
fprintf(stdout, "Print SST file checksum information finished \n");
}

if (!s.ok()) {
exec_state_ = LDBCommandExecuteResult::Failed(s.ToString());
}
}

Expand Down
67 changes: 51 additions & 16 deletions tools/ldb_cmd_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -309,15 +309,20 @@ TEST_F(LdbCmdTest, DumpFileChecksumNoChecksum) {
ASSERT_OK(db->Put(wopts, buf, v));
}
ASSERT_OK(db->Flush(fopts));
ASSERT_OK(db->Close());
delete db;

char arg1[] = "./ldb";
char arg2[1024];
snprintf(arg2, sizeof(arg2), "--db=%s", dbname.c_str());
char arg3[] = "file_checksum_dump";
char* argv[] = {arg1, arg2, arg3};
char arg4[] = "--hex";
char* argv[] = {arg1, arg2, arg3, arg4};

ASSERT_EQ(0,
LDBCommandRunner::RunCommand(3, argv, opts, LDBOptions(), nullptr));
LDBCommandRunner::RunCommand(4, argv, opts, LDBOptions(), nullptr));

ASSERT_OK(DB::Open(opts, dbname, &db));

// Verify each sst file checksum value and checksum name
FileChecksumTestHelper fct_helper(opts, db, dbname);
Expand All @@ -336,8 +341,13 @@ TEST_F(LdbCmdTest, DumpFileChecksumNoChecksum) {
FileChecksumTestHelper fct_helper_ac(opts, db, dbname);
ASSERT_OK(fct_helper_ac.VerifyEachFileChecksum());

ASSERT_OK(db->Close());
delete db;

ASSERT_EQ(0,
LDBCommandRunner::RunCommand(3, argv, opts, LDBOptions(), nullptr));
LDBCommandRunner::RunCommand(4, argv, opts, LDBOptions(), nullptr));

ASSERT_OK(DB::Open(opts, dbname, &db));

// Verify the checksum information in memory is the same as that in Manifest;
std::vector<LiveFileMetaData> live_files;
Expand Down Expand Up @@ -390,14 +400,19 @@ TEST_F(LdbCmdTest, BlobDBDumpFileChecksumNoChecksum) {
ASSERT_OK(db->Put(wopts, oss.str(), v));
}
ASSERT_OK(db->Flush(fopts));
ASSERT_OK(db->Close());
delete db;

char arg1[] = "./ldb";
std::string arg2_str = "--db=" + dbname;
char arg3[] = "file_checksum_dump";
char* argv[] = {arg1, const_cast<char*>(arg2_str.c_str()), arg3};
char arg4[] = "--hex";
char* argv[] = {arg1, const_cast<char*>(arg2_str.c_str()), arg3, arg4};

ASSERT_EQ(0,
LDBCommandRunner::RunCommand(3, argv, opts, LDBOptions(), nullptr));
LDBCommandRunner::RunCommand(4, argv, opts, LDBOptions(), nullptr));

ASSERT_OK(DB::Open(opts, dbname, &db));

// Verify each sst and blob file checksum value and checksum name
FileChecksumTestHelper fct_helper(opts, db, dbname);
Expand All @@ -419,10 +434,11 @@ TEST_F(LdbCmdTest, BlobDBDumpFileChecksumNoChecksum) {
FileChecksumTestHelper fct_helper_ac(opts, db, dbname);
ASSERT_OK(fct_helper_ac.VerifyEachFileChecksum());

ASSERT_EQ(0,
LDBCommandRunner::RunCommand(3, argv, opts, LDBOptions(), nullptr));

ASSERT_OK(db->Close());
delete db;

ASSERT_EQ(0,
LDBCommandRunner::RunCommand(4, argv, opts, LDBOptions(), nullptr));
}

TEST_F(LdbCmdTest, DumpFileChecksumCRC32) {
Expand Down Expand Up @@ -469,15 +485,20 @@ TEST_F(LdbCmdTest, DumpFileChecksumCRC32) {
ASSERT_OK(db->Put(wopts, buf, v));
}
ASSERT_OK(db->Flush(fopts));
ASSERT_OK(db->Close());
delete db;

char arg1[] = "./ldb";
char arg2[1024];
snprintf(arg2, sizeof(arg2), "--db=%s", dbname.c_str());
char arg3[] = "file_checksum_dump";
char* argv[] = {arg1, arg2, arg3};
char arg4[] = "--hex";
char* argv[] = {arg1, arg2, arg3, arg4};

ASSERT_EQ(0,
LDBCommandRunner::RunCommand(3, argv, opts, LDBOptions(), nullptr));
LDBCommandRunner::RunCommand(4, argv, opts, LDBOptions(), nullptr));

ASSERT_OK(DB::Open(opts, dbname, &db));

// Verify each sst file checksum value and checksum name
FileChecksumTestHelper fct_helper(opts, db, dbname);
Expand All @@ -496,14 +517,21 @@ TEST_F(LdbCmdTest, DumpFileChecksumCRC32) {
FileChecksumTestHelper fct_helper_ac(opts, db, dbname);
ASSERT_OK(fct_helper_ac.VerifyEachFileChecksum());

ASSERT_OK(db->Close());
delete db;

ASSERT_EQ(0,
LDBCommandRunner::RunCommand(3, argv, opts, LDBOptions(), nullptr));
LDBCommandRunner::RunCommand(4, argv, opts, LDBOptions(), nullptr));

ASSERT_OK(DB::Open(opts, dbname, &db));

// Verify the checksum information in memory is the same as that in Manifest;
std::vector<LiveFileMetaData> live_files;
db->GetLiveFilesMetaData(&live_files);
delete db;
ASSERT_OK(fct_helper_ac.VerifyChecksumInManifest(live_files));

ASSERT_OK(db->Close());
delete db;
}

TEST_F(LdbCmdTest, BlobDBDumpFileChecksumCRC32) {
Expand Down Expand Up @@ -551,14 +579,19 @@ TEST_F(LdbCmdTest, BlobDBDumpFileChecksumCRC32) {
ASSERT_OK(db->Put(wopts, oss.str(), v));
}
ASSERT_OK(db->Flush(fopts));
ASSERT_OK(db->Close());
delete db;

char arg1[] = "./ldb";
std::string arg2_str = "--db=" + dbname;
char arg3[] = "file_checksum_dump";
char* argv[] = {arg1, const_cast<char*>(arg2_str.c_str()), arg3};
char arg4[] = "--hex";
char* argv[] = {arg1, const_cast<char*>(arg2_str.c_str()), arg3, arg4};

ASSERT_EQ(0,
LDBCommandRunner::RunCommand(3, argv, opts, LDBOptions(), nullptr));
LDBCommandRunner::RunCommand(4, argv, opts, LDBOptions(), nullptr));

ASSERT_OK(DB::Open(opts, dbname, &db));

// Verify each sst and blob file checksum value and checksum name
FileChecksumTestHelper fct_helper(opts, db, dbname);
Expand All @@ -580,9 +613,11 @@ TEST_F(LdbCmdTest, BlobDBDumpFileChecksumCRC32) {
FileChecksumTestHelper fct_helper_ac(opts, db, dbname);
ASSERT_OK(fct_helper_ac.VerifyEachFileChecksum());

ASSERT_EQ(0,
LDBCommandRunner::RunCommand(3, argv, opts, LDBOptions(), nullptr));
ASSERT_OK(db->Close());
delete db;

ASSERT_EQ(0,
LDBCommandRunner::RunCommand(4, argv, opts, LDBOptions(), nullptr));
}

TEST_F(LdbCmdTest, OptionParsing) {
Expand Down

0 comments on commit a5566d5

Please sign in to comment.