Skip to content

Commit

Permalink
Clean up VersionStorageInfo a bit (facebook#9494)
Browse files Browse the repository at this point in the history 
Summary:
The patch does some cleanup in and around `VersionStorageInfo`:
* Renames the method `PrepareApply` to `PrepareAppend` in `Version`
to make it clear that it is to be called before appending the `Version` to
`VersionSet` (via `AppendVersion`), not before applying any `VersionEdit`s.
* Introduces a helper method `VersionStorageInfo::PrepareForVersionAppend`
(called by `Version::PrepareAppend`) that encapsulates the population of the
various derived data structures in `VersionStorageInfo`, and turns the
methods computing the derived structures (`UpdateNumNonEmptyLevels`,
`CalculateBaseBytes` etc.) into private helpers.
* Changes `Version::PrepareAppend` so it only calls `UpdateAccumulatedStats`
if the `update_stats` flag is set. (Earlier, this was checked by the callee.)
Related to this, it also moves the call to `ComputeCompensatedSizes` to
`VersionStorageInfo::PrepareForVersionAppend`.
* Updates and cleans up `version_builder_test`, `version_set_test`, and
`compaction_picker_test` so `PrepareForVersionAppend` is called anytime
a new `VersionStorageInfo` is set up or saved. This cleanup also involves
splitting `VersionStorageInfoTest.MaxBytesForLevelDynamic`
into multiple smaller test cases.
* Fixes up a bunch of comments that were outdated or just plain incorrect.

Pull Request resolved: facebook#9494

Test Plan: Ran `make check` and the crash test script for a while.

Reviewed By: riversand963

Differential Revision: D33971666

Pulled By: ltamasi

fbshipit-source-id: fda52faac7783041126e4f8dec0fe01bdcadf65a
  • Loading branch information
@ltamasi @facebook-github-bot
ltamasi authored and facebook-github-bot committed Feb 4, 2022
1 parent bec9ab4 commit 42e0751
Show file tree
Hide file tree
Showing 8 changed files with 278 additions and 191 deletions.
29 changes: 2 additions & 27 deletions db/compaction/compaction_picker_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ class CompactionPickerTest : public testing::Test {
options_.num_levels = num_levels;
vstorage_.reset(new VersionStorageInfo(&icmp_, ucmp_, options_.num_levels,
style, nullptr, false));
vstorage_->CalculateBaseBytes(ioptions_, mutable_cf_options_);
vstorage_->PrepareForVersionAppend(ioptions_, mutable_cf_options_);
}

// Create a new VersionStorageInfo object so we can add mode files and then
Expand Down Expand Up @@ -148,35 +148,10 @@ class CompactionPickerTest : public testing::Test {
ASSERT_OK(builder.SaveTo(temp_vstorage_.get()));
vstorage_ = std::move(temp_vstorage_);
}
vstorage_->CalculateBaseBytes(ioptions_, mutable_cf_options_);
vstorage_->UpdateFilesByCompactionPri(ioptions_, mutable_cf_options_);
vstorage_->UpdateNumNonEmptyLevels();
vstorage_->GenerateFileIndexer();
vstorage_->GenerateLevelFilesBrief();
vstorage_->PrepareForVersionAppend(ioptions_, mutable_cf_options_);
vstorage_->ComputeCompactionScore(ioptions_, mutable_cf_options_);
vstorage_->GenerateLevel0NonOverlapping();
vstorage_->ComputeFilesMarkedForCompaction();
vstorage_->SetFinalized();
}
void AddFileToVersionStorage(int level, uint32_t file_number,
const char* smallest, const char* largest,
uint64_t file_size = 1, uint32_t path_id = 0,
SequenceNumber smallest_seq = 100,
SequenceNumber largest_seq = 100,
size_t compensated_file_size = 0,
bool marked_for_compact = false) {
VersionStorageInfo* base_vstorage = vstorage_.release();
vstorage_.reset(new VersionStorageInfo(&icmp_, ucmp_, options_.num_levels,
kCompactionStyleUniversal,
base_vstorage, false));
Add(level, file_number, smallest, largest, file_size, path_id, smallest_seq,
largest_seq, compensated_file_size, marked_for_compact);

VersionBuilder builder(FileOptions(), &ioptions_, nullptr, base_vstorage,
nullptr);
builder.SaveTo(vstorage_.get());
UpdateVersionStorageInfo();
}

private:
std::unique_ptr<VersionStorageInfo> temp_vstorage_;
Expand Down
90 changes: 79 additions & 11 deletions db/version_builder_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,15 +152,14 @@ class VersionBuilderTest : public testing::Test {
return meta;
}

void UpdateVersionStorageInfo() {
vstorage_.UpdateFilesByCompactionPri(ioptions_, mutable_cf_options_);
vstorage_.UpdateNumNonEmptyLevels();
vstorage_.GenerateFileIndexer();
vstorage_.GenerateLevelFilesBrief();
vstorage_.CalculateBaseBytes(ioptions_, mutable_cf_options_);
vstorage_.GenerateLevel0NonOverlapping();
vstorage_.SetFinalized();
void UpdateVersionStorageInfo(VersionStorageInfo* vstorage) {
assert(vstorage);

vstorage->PrepareForVersionAppend(ioptions_, mutable_cf_options_);
vstorage->SetFinalized();
}

void UpdateVersionStorageInfo() { UpdateVersionStorageInfo(&vstorage_); }
};

void UnrefFilesInVersion(VersionStorageInfo* new_vstorage) {
Expand All @@ -187,6 +186,7 @@ TEST_F(VersionBuilderTest, ApplyAndSaveTo) {
Add(3, 27U, "171", "179", 100U);
Add(3, 28U, "191", "220", 100U);
Add(3, 29U, "221", "300", 100U);

UpdateVersionStorageInfo();

VersionEdit version_edit;
Expand All @@ -210,6 +210,8 @@ TEST_F(VersionBuilderTest, ApplyAndSaveTo) {
ASSERT_OK(version_builder.Apply(&version_edit));
ASSERT_OK(version_builder.SaveTo(&new_vstorage));

UpdateVersionStorageInfo(&new_vstorage);

ASSERT_EQ(400U, new_vstorage.NumLevelBytes(2));
ASSERT_EQ(300U, new_vstorage.NumLevelBytes(3));

Expand All @@ -228,6 +230,7 @@ TEST_F(VersionBuilderTest, ApplyAndSaveToDynamic) {

Add(5, 26U, "150", "170", 100U);
Add(5, 27U, "171", "179", 100U);

UpdateVersionStorageInfo();

VersionEdit version_edit;
Expand All @@ -252,6 +255,8 @@ TEST_F(VersionBuilderTest, ApplyAndSaveToDynamic) {
ASSERT_OK(version_builder.Apply(&version_edit));
ASSERT_OK(version_builder.SaveTo(&new_vstorage));

UpdateVersionStorageInfo(&new_vstorage);

ASSERT_EQ(0U, new_vstorage.NumLevelBytes(0));
ASSERT_EQ(100U, new_vstorage.NumLevelBytes(3));
ASSERT_EQ(300U, new_vstorage.NumLevelBytes(4));
Expand All @@ -272,6 +277,7 @@ TEST_F(VersionBuilderTest, ApplyAndSaveToDynamic2) {

Add(5, 26U, "150", "170", 100U);
Add(5, 27U, "171", "179", 100U);

UpdateVersionStorageInfo();

VersionEdit version_edit;
Expand Down Expand Up @@ -299,6 +305,8 @@ TEST_F(VersionBuilderTest, ApplyAndSaveToDynamic2) {
ASSERT_OK(version_builder.Apply(&version_edit));
ASSERT_OK(version_builder.SaveTo(&new_vstorage));

UpdateVersionStorageInfo(&new_vstorage);

ASSERT_EQ(0U, new_vstorage.NumLevelBytes(0));
ASSERT_EQ(100U, new_vstorage.NumLevelBytes(4));
ASSERT_EQ(200U, new_vstorage.NumLevelBytes(5));
Expand Down Expand Up @@ -353,6 +361,8 @@ TEST_F(VersionBuilderTest, ApplyMultipleAndSaveTo) {
ASSERT_OK(version_builder.Apply(&version_edit));
ASSERT_OK(version_builder.SaveTo(&new_vstorage));

UpdateVersionStorageInfo(&new_vstorage);

ASSERT_EQ(500U, new_vstorage.NumLevelBytes(2));

UnrefFilesInVersion(&new_vstorage);
Expand Down Expand Up @@ -423,6 +433,8 @@ TEST_F(VersionBuilderTest, ApplyDeleteAndSaveTo) {
ASSERT_OK(version_builder.Apply(&version_edit2));
ASSERT_OK(version_builder.SaveTo(&new_vstorage));

UpdateVersionStorageInfo(&new_vstorage);

ASSERT_EQ(300U, new_vstorage.NumLevelBytes(2));

UnrefFilesInVersion(&new_vstorage);
Expand All @@ -433,8 +445,11 @@ TEST_F(VersionBuilderTest, ApplyFileDeletionIncorrectLevel) {
constexpr uint64_t file_number = 2345;
constexpr char smallest[] = "bar";
constexpr char largest[] = "foo";
constexpr uint64_t file_size = 100;

Add(level, file_number, smallest, largest);
Add(level, file_number, smallest, largest, file_size);

UpdateVersionStorageInfo();

EnvOptions env_options;
constexpr TableCache* table_cache = nullptr;
Expand All @@ -457,6 +472,8 @@ TEST_F(VersionBuilderTest, ApplyFileDeletionIncorrectLevel) {
}

TEST_F(VersionBuilderTest, ApplyFileDeletionNotInLSMTree) {
UpdateVersionStorageInfo();

EnvOptions env_options;
constexpr TableCache* table_cache = nullptr;
constexpr VersionSet* version_set = nullptr;
Expand Down Expand Up @@ -497,6 +514,8 @@ TEST_F(VersionBuilderTest, ApplyFileDeletionAndAddition) {
largest_seq, num_entries, num_deletions, sampled, smallest_seqno,
largest_seqno);

UpdateVersionStorageInfo();

EnvOptions env_options;
constexpr TableCache* table_cache = nullptr;
constexpr VersionSet* version_set = nullptr;
Expand Down Expand Up @@ -531,6 +550,9 @@ TEST_F(VersionBuilderTest, ApplyFileDeletionAndAddition) {
force_consistency_checks);

ASSERT_OK(builder.SaveTo(&new_vstorage));

UpdateVersionStorageInfo(&new_vstorage);

ASSERT_EQ(new_vstorage.GetFileLocation(file_number).GetLevel(), level);

UnrefFilesInVersion(&new_vstorage);
Expand All @@ -541,8 +563,11 @@ TEST_F(VersionBuilderTest, ApplyFileAdditionAlreadyInBase) {
constexpr uint64_t file_number = 2345;
constexpr char smallest[] = "bar";
constexpr char largest[] = "foo";
constexpr uint64_t file_size = 10000;

Add(level, file_number, smallest, largest, file_size);

Add(level, file_number, smallest, largest);
UpdateVersionStorageInfo();

EnvOptions env_options;
constexpr TableCache* table_cache = nullptr;
Expand All @@ -555,7 +580,6 @@ TEST_F(VersionBuilderTest, ApplyFileAdditionAlreadyInBase) {

constexpr int new_level = 2;
constexpr uint32_t path_id = 0;
constexpr uint64_t file_size = 10000;
constexpr SequenceNumber smallest_seqno = 100;
constexpr SequenceNumber largest_seqno = 1000;
constexpr bool marked_for_compaction = false;
Expand All @@ -576,6 +600,8 @@ TEST_F(VersionBuilderTest, ApplyFileAdditionAlreadyInBase) {
}

TEST_F(VersionBuilderTest, ApplyFileAdditionAlreadyApplied) {
UpdateVersionStorageInfo();

EnvOptions env_options;
constexpr TableCache* table_cache = nullptr;
constexpr VersionSet* version_set = nullptr;
Expand Down Expand Up @@ -625,6 +651,8 @@ TEST_F(VersionBuilderTest, ApplyFileAdditionAlreadyApplied) {
}

TEST_F(VersionBuilderTest, ApplyFileAdditionAndDeletion) {
UpdateVersionStorageInfo();

constexpr int level = 1;
constexpr uint64_t file_number = 2345;
constexpr uint32_t path_id = 0;
Expand Down Expand Up @@ -666,12 +694,17 @@ TEST_F(VersionBuilderTest, ApplyFileAdditionAndDeletion) {
force_consistency_checks);

ASSERT_OK(builder.SaveTo(&new_vstorage));

UpdateVersionStorageInfo(&new_vstorage);

ASSERT_FALSE(new_vstorage.GetFileLocation(file_number).IsValid());

UnrefFilesInVersion(&new_vstorage);
}

TEST_F(VersionBuilderTest, ApplyBlobFileAddition) {
UpdateVersionStorageInfo();

EnvOptions env_options;
constexpr TableCache* table_cache = nullptr;
constexpr VersionSet* version_set = nullptr;
Expand Down Expand Up @@ -705,6 +738,8 @@ TEST_F(VersionBuilderTest, ApplyBlobFileAddition) {

ASSERT_OK(builder.SaveTo(&new_vstorage));

UpdateVersionStorageInfo(&new_vstorage);

const auto& new_blob_files = new_vstorage.GetBlobFiles();
ASSERT_EQ(new_blob_files.size(), 1);

Expand Down Expand Up @@ -741,6 +776,8 @@ TEST_F(VersionBuilderTest, ApplyBlobFileAdditionAlreadyInBase) {
checksum_value, BlobFileMetaData::LinkedSsts(), garbage_blob_count,
garbage_blob_bytes);

UpdateVersionStorageInfo();

EnvOptions env_options;
constexpr TableCache* table_cache = nullptr;
constexpr VersionSet* version_set = nullptr;
Expand All @@ -761,6 +798,8 @@ TEST_F(VersionBuilderTest, ApplyBlobFileAdditionAlreadyInBase) {
TEST_F(VersionBuilderTest, ApplyBlobFileAdditionAlreadyApplied) {
// Attempt to add the same blob file twice using version edits.

UpdateVersionStorageInfo();

EnvOptions env_options;
constexpr TableCache* table_cache = nullptr;
constexpr VersionSet* version_set = nullptr;
Expand Down Expand Up @@ -813,6 +852,8 @@ TEST_F(VersionBuilderTest, ApplyBlobFileGarbageFileInBase) {
// Add dummy table file to ensure the blob file is referenced.
AddDummyFile(table_file_number, blob_file_number);

UpdateVersionStorageInfo();

EnvOptions env_options;
constexpr TableCache* table_cache = nullptr;
constexpr VersionSet* version_set = nullptr;
Expand All @@ -837,6 +878,8 @@ TEST_F(VersionBuilderTest, ApplyBlobFileGarbageFileInBase) {

ASSERT_OK(builder.SaveTo(&new_vstorage));

UpdateVersionStorageInfo(&new_vstorage);

const auto& new_blob_files = new_vstorage.GetBlobFiles();
ASSERT_EQ(new_blob_files.size(), 1);

Expand All @@ -862,6 +905,8 @@ TEST_F(VersionBuilderTest, ApplyBlobFileGarbageFileInBase) {
TEST_F(VersionBuilderTest, ApplyBlobFileGarbageFileAdditionApplied) {
// Increase the amount of garbage for a blob file added using a version edit.

UpdateVersionStorageInfo();

EnvOptions env_options;
constexpr TableCache* table_cache = nullptr;
constexpr VersionSet* version_set = nullptr;
Expand Down Expand Up @@ -905,6 +950,8 @@ TEST_F(VersionBuilderTest, ApplyBlobFileGarbageFileAdditionApplied) {

ASSERT_OK(builder.SaveTo(&new_vstorage));

UpdateVersionStorageInfo(&new_vstorage);

const auto& new_blob_files = new_vstorage.GetBlobFiles();
ASSERT_EQ(new_blob_files.size(), 1);

Expand All @@ -928,6 +975,8 @@ TEST_F(VersionBuilderTest, ApplyBlobFileGarbageFileNotFound) {
// Attempt to increase the amount of garbage for a blob file that is
// neither in the base version, nor was it added using a version edit.

UpdateVersionStorageInfo();

EnvOptions env_options;
constexpr TableCache* table_cache = nullptr;
constexpr VersionSet* version_set = nullptr;
Expand All @@ -953,6 +1002,8 @@ TEST_F(VersionBuilderTest, BlobFileGarbageOverflow) {
// Test that VersionEdits that would result in the count/total size of garbage
// exceeding the count/total size of all blobs are rejected.

UpdateVersionStorageInfo();

EnvOptions env_options;
constexpr TableCache* table_cache = nullptr;
constexpr VersionSet* version_set = nullptr;
Expand Down Expand Up @@ -1032,6 +1083,8 @@ TEST_F(VersionBuilderTest, SaveBlobFilesTo) {
AddDummyFile(table_file_number, blob_file_number);
}

UpdateVersionStorageInfo();

EnvOptions env_options;
constexpr TableCache* table_cache = nullptr;
constexpr VersionSet* version_set = nullptr;
Expand Down Expand Up @@ -1069,6 +1122,8 @@ TEST_F(VersionBuilderTest, SaveBlobFilesTo) {

ASSERT_OK(builder.SaveTo(&new_vstorage));

UpdateVersionStorageInfo(&new_vstorage);

const auto& new_blob_files = new_vstorage.GetBlobFiles();
ASSERT_EQ(new_blob_files.size(), 3);

Expand Down Expand Up @@ -1115,6 +1170,8 @@ TEST_F(VersionBuilderTest, SaveBlobFilesTo) {

ASSERT_OK(second_builder.SaveTo(&newer_vstorage));

UpdateVersionStorageInfo(&newer_vstorage);

const auto& newer_blob_files = newer_vstorage.GetBlobFiles();
ASSERT_EQ(newer_blob_files.size(), 2);

Expand Down Expand Up @@ -1149,6 +1206,8 @@ TEST_F(VersionBuilderTest, SaveBlobFilesToConcurrentJobs) {
BlobFileMetaData::LinkedSsts{base_table_file_number},
garbage_blob_count, garbage_blob_bytes);

UpdateVersionStorageInfo();

EnvOptions env_options;
constexpr TableCache* table_cache = nullptr;
constexpr VersionSet* version_set = nullptr;
Expand Down Expand Up @@ -1194,6 +1253,8 @@ TEST_F(VersionBuilderTest, SaveBlobFilesToConcurrentJobs) {

ASSERT_OK(builder.SaveTo(&new_vstorage));

UpdateVersionStorageInfo(&new_vstorage);

const auto& new_blob_files = new_vstorage.GetBlobFiles();
ASSERT_EQ(new_blob_files.size(), 2);

Expand Down Expand Up @@ -1293,6 +1354,8 @@ TEST_F(VersionBuilderTest, CheckConsistencyForBlobFiles) {

ASSERT_OK(builder.SaveTo(&new_vstorage));

UpdateVersionStorageInfo(&new_vstorage);

UnrefFilesInVersion(&new_vstorage);
}

Expand Down Expand Up @@ -1581,6 +1644,8 @@ TEST_F(VersionBuilderTest, MaintainLinkedSstsForBlobFiles) {

ASSERT_OK(builder.SaveTo(&new_vstorage));

UpdateVersionStorageInfo(&new_vstorage);

{
const auto& blob_files = new_vstorage.GetBlobFiles();
ASSERT_EQ(blob_files.size(), 5);
Expand All @@ -1605,6 +1670,7 @@ TEST_F(VersionBuilderTest, MaintainLinkedSstsForBlobFiles) {

TEST_F(VersionBuilderTest, CheckConsistencyForFileDeletedTwice) {
Add(0, 1U, "150", "200", 100U);

UpdateVersionStorageInfo();

VersionEdit version_edit;
Expand All @@ -1622,6 +1688,8 @@ TEST_F(VersionBuilderTest, CheckConsistencyForFileDeletedTwice) {
ASSERT_OK(version_builder.Apply(&version_edit));
ASSERT_OK(version_builder.SaveTo(&new_vstorage));

UpdateVersionStorageInfo(&new_vstorage);

VersionBuilder version_builder2(env_options, &ioptions_, table_cache,
&new_vstorage, version_set);
VersionStorageInfo new_vstorage2(&icmp_, ucmp_, options_.num_levels,
Expand Down
Loading

0 comments on commit 42e0751

Please sign in to comment.