andreacavalli/rocksdb
0
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Fix flaky, dubious LdbCmdTest::*DumpFileChecksum* (#8898)

Browse Source 
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: https://github.com/facebook/rocksdb/pull/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
This commit is contained in:
Peter Dillinger 2021-09-13 17:06:05 -07:00 committed by Facebook GitHub Bot
parent 7bef598440
commit a5566d508b
2 changed files with 65 additions and 28 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

26
tools/ldb_cmd.cc
View File

@ -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);
@ -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
@ -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());
@ -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());
}
}

67
tools/ldb_cmd_test.cc
View File

@ -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);
@ -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;
@ -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);
@ -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) {
@ -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);
@ -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) {
@ -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);
@ -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) {