From de00f28132bd91593f9fd1d565189f7cfe81918b Mon Sep 17 00:00:00 2001 From: Yanqin Jin Date: Mon, 8 Apr 2019 11:12:25 -0700 Subject: [PATCH] Refactor ExternalSSTFileTest (#5129) Summary: remove an unnecessary function `GenerateAndAddFileIngestBehind` Pull Request resolved: https://github.com/facebook/rocksdb/pull/5129 Differential Revision: D14686710 Pulled By: riversand963 fbshipit-source-id: 5698ae63e10f8ef76c2da753bbb07a36024ac065 --- db/external_sst_file_test.cc | 176 ++++++++++++----------------------- 1 file changed, 62 insertions(+), 114 deletions(-) diff --git a/db/external_sst_file_test.cc b/db/external_sst_file_test.cc index 599fe5786..cbbb2fa26 100644 --- a/db/external_sst_file_test.cc +++ b/db/external_sst_file_test.cc @@ -83,7 +83,8 @@ class ExternalSSTFileTest const Options options, std::vector> data, int file_id = -1, bool allow_global_seqno = false, bool write_global_seqno = false, - bool verify_checksums_before_ingest = true, bool sort_data = false, + bool verify_checksums_before_ingest = true, bool ingest_behind = false, + bool sort_data = false, std::map* true_data = nullptr, ColumnFamilyHandle* cfh = nullptr) { // Generate a file id if not provided @@ -128,66 +129,7 @@ class ExternalSSTFileTest ifo.allow_global_seqno = allow_global_seqno; ifo.write_global_seqno = allow_global_seqno ? write_global_seqno : false; ifo.verify_checksums_before_ingest = verify_checksums_before_ingest; - if (cfh) { - s = db_->IngestExternalFile(cfh, {file_path}, ifo); - } else { - s = db_->IngestExternalFile({file_path}, ifo); - } - } - - if (s.ok() && true_data) { - for (auto& entry : data) { - (*true_data)[entry.first] = entry.second; - } - } - - return s; - } - - Status GenerateAndAddExternalFileIngestBehind( - const Options options, const IngestExternalFileOptions ifo, - std::vector> data, int file_id = -1, - bool sort_data = false, - std::map* true_data = nullptr, - ColumnFamilyHandle* cfh = nullptr) { - // Generate a file id if not provided - if (file_id == -1) { - file_id = last_file_id_ + 1; - last_file_id_++; - } - - // Sort data if asked to do so - if (sort_data) { - std::sort(data.begin(), data.end(), - [&](const std::pair& e1, - const std::pair& e2) { - return options.comparator->Compare(e1.first, e2.first) < 0; - }); - auto uniq_iter = std::unique( - data.begin(), data.end(), - [&](const std::pair& e1, - const std::pair& e2) { - return options.comparator->Compare(e1.first, e2.first) == 0; - }); - data.resize(uniq_iter - data.begin()); - } - std::string file_path = sst_files_dir_ + ToString(file_id); - SstFileWriter sst_file_writer(EnvOptions(), options, cfh); - - Status s = sst_file_writer.Open(file_path); - if (!s.ok()) { - return s; - } - for (auto& entry : data) { - s = sst_file_writer.Put(entry.first, entry.second); - if (!s.ok()) { - sst_file_writer.Finish(); - return s; - } - } - s = sst_file_writer.Finish(); - - if (s.ok()) { + ifo.ingest_behind = ingest_behind; if (cfh) { s = db_->IngestExternalFile(cfh, {file_path}, ifo); } else { @@ -243,31 +185,35 @@ class ExternalSSTFileTest const Options options, std::vector> data, int file_id = -1, bool allow_global_seqno = false, bool write_global_seqno = false, - bool verify_checksums_before_ingest = true, bool sort_data = false, + bool verify_checksums_before_ingest = true, bool ingest_behind = false, + bool sort_data = false, std::map* true_data = nullptr, ColumnFamilyHandle* cfh = nullptr) { std::vector> file_data; for (auto& entry : data) { file_data.emplace_back(Key(entry.first), entry.second); } - return GenerateAndAddExternalFile( - options, file_data, file_id, allow_global_seqno, write_global_seqno, - verify_checksums_before_ingest, sort_data, true_data, cfh); + return GenerateAndAddExternalFile(options, file_data, file_id, + allow_global_seqno, write_global_seqno, + verify_checksums_before_ingest, + ingest_behind, sort_data, true_data, cfh); } Status GenerateAndAddExternalFile( const Options options, std::vector keys, int file_id = -1, bool allow_global_seqno = false, bool write_global_seqno = false, - bool verify_checksums_before_ingest = true, bool sort_data = false, + bool verify_checksums_before_ingest = true, bool ingest_behind = false, + bool sort_data = false, std::map* true_data = nullptr, ColumnFamilyHandle* cfh = nullptr) { std::vector> file_data; for (auto& k : keys) { file_data.emplace_back(Key(k), Key(k) + ToString(file_id)); } - return GenerateAndAddExternalFile( - options, file_data, file_id, allow_global_seqno, write_global_seqno, - verify_checksums_before_ingest, sort_data, true_data, cfh); + return GenerateAndAddExternalFile(options, file_data, file_id, + allow_global_seqno, write_global_seqno, + verify_checksums_before_ingest, + ingest_behind, sort_data, true_data, cfh); } Status DeprecatedAddFile(const std::vector& files, @@ -1246,12 +1192,12 @@ TEST_P(ExternalSSTFileTest, PickedLevel) { // File 0 will go to last level (L3) ASSERT_OK(GenerateAndAddExternalFile(options, {1, 10}, -1, false, false, true, - false, &true_data)); + false, false, &true_data)); EXPECT_EQ(FilesPerLevel(), "0,0,0,1"); // File 1 will go to level L2 (since it overlap with file 0 in L3) ASSERT_OK(GenerateAndAddExternalFile(options, {2, 9}, -1, false, false, true, - false, &true_data)); + false, false, &true_data)); EXPECT_EQ(FilesPerLevel(), "0,0,1,1"); rocksdb::SyncPoint::GetInstance()->LoadDependency({ @@ -1281,12 +1227,12 @@ TEST_P(ExternalSSTFileTest, PickedLevel) { // This file overlaps with file 0 (L3), file 1 (L2) and the // output of compaction going to L1 ASSERT_OK(GenerateAndAddExternalFile(options, {4, 7}, -1, false, false, true, - false, &true_data)); + false, false, &true_data)); EXPECT_EQ(FilesPerLevel(), "5,0,1,1"); // This file does not overlap with any file or with the running compaction ASSERT_OK(GenerateAndAddExternalFile(options, {9000, 9001}, -1, false, false, - true, false, &true_data)); + false, false, false, &true_data)); EXPECT_EQ(FilesPerLevel(), "5,0,1,2"); // Hold compaction from finishing @@ -1517,12 +1463,12 @@ TEST_F(ExternalSSTFileTest, PickedLevelDynamic) { // This file overlaps with the output of the compaction (going to L3) // so the file will be added to L0 since L3 is the base level ASSERT_OK(GenerateAndAddExternalFile(options, {31, 32, 33, 34}, -1, false, - false, true, false, &true_data)); + false, true, false, false, &true_data)); EXPECT_EQ(FilesPerLevel(), "5"); // This file does not overlap with the current running compactiong ASSERT_OK(GenerateAndAddExternalFile(options, {9000, 9001}, -1, false, false, - true, false, &true_data)); + true, false, false, &true_data)); EXPECT_EQ(FilesPerLevel(), "5,0,0,1"); // Hold compaction from finishing @@ -1537,25 +1483,25 @@ TEST_F(ExternalSSTFileTest, PickedLevelDynamic) { Reopen(options); ASSERT_OK(GenerateAndAddExternalFile(options, {1, 15, 19}, -1, false, false, - true, false, &true_data)); + true, false, false, &true_data)); ASSERT_EQ(FilesPerLevel(), "1,0,0,3"); ASSERT_OK(GenerateAndAddExternalFile(options, {1000, 1001, 1002}, -1, false, - false, true, false, &true_data)); + false, true, false, false, &true_data)); ASSERT_EQ(FilesPerLevel(), "1,0,0,4"); ASSERT_OK(GenerateAndAddExternalFile(options, {500, 600, 700}, -1, false, - false, true, false, &true_data)); + false, true, false, false, &true_data)); ASSERT_EQ(FilesPerLevel(), "1,0,0,5"); // File 5 overlaps with file 2 (L3 / base level) ASSERT_OK(GenerateAndAddExternalFile(options, {2, 10}, -1, false, false, true, - false, &true_data)); + false, false, &true_data)); ASSERT_EQ(FilesPerLevel(), "2,0,0,5"); // File 6 overlaps with file 2 (L3 / base level) and file 5 (L0) ASSERT_OK(GenerateAndAddExternalFile(options, {3, 9}, -1, false, false, true, - false, &true_data)); + false, false, &true_data)); ASSERT_EQ(FilesPerLevel(), "3,0,0,5"); // Verify data in files @@ -1574,7 +1520,7 @@ TEST_F(ExternalSSTFileTest, PickedLevelDynamic) { // File 7 overlaps with file 4 (L3) ASSERT_OK(GenerateAndAddExternalFile(options, {650, 651, 652}, -1, false, - false, true, false, &true_data)); + false, true, false, false, &true_data)); ASSERT_EQ(FilesPerLevel(), "5,0,0,5"); VerifyDBFromMap(true_data, &kcnt, false); @@ -1741,7 +1687,7 @@ TEST_P(ExternalSSTFileTest, IngestFileWithGlobalSeqnoRandomized) { } else { ASSERT_OK(GenerateAndAddExternalFile( options, random_data, -1, true, write_global_seqno, - verify_checksums_before_ingest, true, &true_data)); + verify_checksums_before_ingest, false, true, &true_data)); } } size_t kcnt = 0; @@ -1774,7 +1720,7 @@ TEST_P(ExternalSSTFileTest, IngestFileWithGlobalSeqnoAssignedLevel) { bool verify_checksums_before_ingest = std::get<1>(GetParam()); ASSERT_OK(GenerateAndAddExternalFile( options, file_data, -1, true, write_global_seqno, - verify_checksums_before_ingest, false, &true_data)); + verify_checksums_before_ingest, false, false, &true_data)); // This file dont overlap with anything in the DB, will go to L4 ASSERT_EQ("0,0,0,0,1", FilesPerLevel()); @@ -1786,7 +1732,7 @@ TEST_P(ExternalSSTFileTest, IngestFileWithGlobalSeqnoAssignedLevel) { } ASSERT_OK(GenerateAndAddExternalFile( options, file_data, -1, true, write_global_seqno, - verify_checksums_before_ingest, false, &true_data)); + verify_checksums_before_ingest, false, false, &true_data)); // This file overlap with the memtable, so it will flush it and add // it self to L0 @@ -1799,7 +1745,7 @@ TEST_P(ExternalSSTFileTest, IngestFileWithGlobalSeqnoAssignedLevel) { } ASSERT_OK(GenerateAndAddExternalFile( options, file_data, -1, true, write_global_seqno, - verify_checksums_before_ingest, false, &true_data)); + verify_checksums_before_ingest, false, false, &true_data)); // This file dont overlap with anything in the DB and fit in L4 as well ASSERT_EQ("2,0,0,0,2", FilesPerLevel()); @@ -1811,7 +1757,7 @@ TEST_P(ExternalSSTFileTest, IngestFileWithGlobalSeqnoAssignedLevel) { } ASSERT_OK(GenerateAndAddExternalFile( options, file_data, -1, true, write_global_seqno, - verify_checksums_before_ingest, false, &true_data)); + verify_checksums_before_ingest, false, false, &true_data)); // This file overlap with files in L4, we will ingest it in L3 ASSERT_EQ("2,0,0,1,2", FilesPerLevel()); @@ -1839,7 +1785,7 @@ TEST_P(ExternalSSTFileTest, IngestFileWithGlobalSeqnoMemtableFlush) { // No need for flush ASSERT_OK(GenerateAndAddExternalFile( options, {90, 100, 110}, -1, true, write_global_seqno, - verify_checksums_before_ingest, false, &true_data)); + verify_checksums_before_ingest, false, false, &true_data)); db_->GetIntProperty(DB::Properties::kNumEntriesActiveMemTable, &entries_in_memtable); ASSERT_GE(entries_in_memtable, 1); @@ -1847,7 +1793,7 @@ TEST_P(ExternalSSTFileTest, IngestFileWithGlobalSeqnoMemtableFlush) { // This file will flush the memtable ASSERT_OK(GenerateAndAddExternalFile( options, {19, 20, 21}, -1, true, write_global_seqno, - verify_checksums_before_ingest, false, &true_data)); + verify_checksums_before_ingest, false, false, &true_data)); db_->GetIntProperty(DB::Properties::kNumEntriesActiveMemTable, &entries_in_memtable); ASSERT_EQ(entries_in_memtable, 0); @@ -1863,15 +1809,15 @@ TEST_P(ExternalSSTFileTest, IngestFileWithGlobalSeqnoMemtableFlush) { // No need for flush, this file keys fit between the memtable keys ASSERT_OK(GenerateAndAddExternalFile( options, {202, 203, 204}, -1, true, write_global_seqno, - verify_checksums_before_ingest, false, &true_data)); + verify_checksums_before_ingest, false, false, &true_data)); db_->GetIntProperty(DB::Properties::kNumEntriesActiveMemTable, &entries_in_memtable); ASSERT_GE(entries_in_memtable, 1); // This file will flush the memtable ASSERT_OK(GenerateAndAddExternalFile( - options, {206, 207}, -1, true, false, write_global_seqno, - verify_checksums_before_ingest, &true_data)); + options, {206, 207}, -1, true, write_global_seqno, + verify_checksums_before_ingest, false, false, &true_data)); db_->GetIntProperty(DB::Properties::kNumEntriesActiveMemTable, &entries_in_memtable); ASSERT_EQ(entries_in_memtable, 0); @@ -1892,13 +1838,13 @@ TEST_P(ExternalSSTFileTest, L0SortingIssue) { bool write_global_seqno = std::get<0>(GetParam()); bool verify_checksums_before_ingest = std::get<1>(GetParam()); // No Flush needed, No global seqno needed, Ingest in L1 - ASSERT_OK(GenerateAndAddExternalFile(options, {7, 8}, -1, true, - write_global_seqno, - verify_checksums_before_ingest, false)); + ASSERT_OK( + GenerateAndAddExternalFile(options, {7, 8}, -1, true, write_global_seqno, + verify_checksums_before_ingest, false, false)); // No Flush needed, but need a global seqno, Ingest in L0 - ASSERT_OK(GenerateAndAddExternalFile(options, {7, 8}, -1, true, - write_global_seqno, - verify_checksums_before_ingest, false)); + ASSERT_OK( + GenerateAndAddExternalFile(options, {7, 8}, -1, true, write_global_seqno, + verify_checksums_before_ingest, false, false)); printf("%s\n", FilesPerLevel().c_str()); // Overwrite what we added using external files @@ -2139,7 +2085,7 @@ TEST_P(ExternalSSTFileTest, IngestionListener) { // Ingest into default cf ASSERT_OK(GenerateAndAddExternalFile( options, {1, 2}, -1, true, write_global_seqno, - verify_checksums_before_ingest, true, nullptr, handles_[0])); + verify_checksums_before_ingest, false, true, nullptr, handles_[0])); ASSERT_EQ(listener->ingested_files.size(), 1); ASSERT_EQ(listener->ingested_files.back().cf_name, "default"); ASSERT_EQ(listener->ingested_files.back().global_seqno, 0); @@ -2151,7 +2097,7 @@ TEST_P(ExternalSSTFileTest, IngestionListener) { // Ingest into cf1 ASSERT_OK(GenerateAndAddExternalFile( options, {1, 2}, -1, true, write_global_seqno, - verify_checksums_before_ingest, true, nullptr, handles_[1])); + verify_checksums_before_ingest, false, true, nullptr, handles_[1])); ASSERT_EQ(listener->ingested_files.size(), 2); ASSERT_EQ(listener->ingested_files.back().cf_name, "koko"); ASSERT_EQ(listener->ingested_files.back().global_seqno, 0); @@ -2163,7 +2109,7 @@ TEST_P(ExternalSSTFileTest, IngestionListener) { // Ingest into cf2 ASSERT_OK(GenerateAndAddExternalFile( options, {1, 2}, -1, true, write_global_seqno, - verify_checksums_before_ingest, true, nullptr, handles_[2])); + verify_checksums_before_ingest, false, true, nullptr, handles_[2])); ASSERT_EQ(listener->ingested_files.size(), 3); ASSERT_EQ(listener->ingested_files.back().cf_name, "toto"); ASSERT_EQ(listener->ingested_files.back().global_seqno, 0); @@ -2226,16 +2172,16 @@ TEST_P(ExternalSSTFileTest, IngestBehind) { file_data.emplace_back(Key(i), "ingest_behind"); } - IngestExternalFileOptions ifo; - ifo.allow_global_seqno = true; - ifo.ingest_behind = true; - ifo.write_global_seqno = std::get<0>(GetParam()); - ifo.verify_checksums_before_ingest = std::get<1>(GetParam()); + bool allow_global_seqno = true; + bool ingest_behind = true; + bool write_global_seqno = std::get<0>(GetParam()); + bool verify_checksums_before_ingest = std::get<1>(GetParam()); // Can't ingest behind since allow_ingest_behind isn't set to true - ASSERT_NOK(GenerateAndAddExternalFileIngestBehind(options, ifo, - file_data, -1, false, - &true_data)); + ASSERT_NOK(GenerateAndAddExternalFile( + options, file_data, -1, allow_global_seqno, write_global_seqno, + verify_checksums_before_ingest, ingest_behind, false /*sort_data*/, + &true_data)); options.allow_ingest_behind = true; // check that we still can open the DB, as num_levels should be @@ -2253,14 +2199,16 @@ TEST_P(ExternalSSTFileTest, IngestBehind) { db_->CompactRange(CompactRangeOptions(), nullptr, nullptr); // Universal picker should go at second from the bottom level ASSERT_EQ("0,1", FilesPerLevel()); - ASSERT_OK(GenerateAndAddExternalFileIngestBehind(options, ifo, - file_data, -1, false, - &true_data)); + ASSERT_OK(GenerateAndAddExternalFile( + options, file_data, -1, allow_global_seqno, write_global_seqno, + verify_checksums_before_ingest, true /*ingest_behind*/, + false /*sort_data*/, &true_data)); ASSERT_EQ("0,1,1", FilesPerLevel()); // this time ingest should fail as the file doesn't fit to the bottom level - ASSERT_NOK(GenerateAndAddExternalFileIngestBehind(options, ifo, - file_data, -1, false, - &true_data)); + ASSERT_NOK(GenerateAndAddExternalFile( + options, file_data, -1, allow_global_seqno, write_global_seqno, + verify_checksums_before_ingest, true /*ingest_behind*/, + false /*sort_data*/, &true_data)); ASSERT_EQ("0,1,1", FilesPerLevel()); db_->CompactRange(CompactRangeOptions(), nullptr, nullptr); // bottom level should be empty