Skip to content

Commit

Permalink
Fix compaction bug in Cuckoo Table Builder. Use kvs_.size() instead o…
Browse files Browse the repository at this point in the history 
…f num_entries in FileSize() method.

Summary: Fix compaction bug in Cuckoo Table Builder. Use kvs_.size() instead of num_entries in FileSize() method. Also added tests.

Test Plan:
make check all
Also ran db_bench to generate multiple files.

Reviewers: sdong, ljin

Reviewed By: ljin

Subscribers: leveldb

Differential Revision: https://reviews.facebook.net/D22743
  • Loading branch information
@adyarshyam
adyarshyam committed Sep 5, 2014
1 parent 0fbb3fa commit 5cd0576
Show file tree
Hide file tree
Showing 3 changed files with 60 additions and 12 deletions.
26 changes: 25 additions & 1 deletion db/cuckoo_table_db_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -245,14 +245,38 @@ TEST(CuckooTableDBTest, CompactionTrigger) {
ASSERT_OK(Put(Key(idx), std::string(10000, 'a' + idx)));
}
dbfull()->TEST_WaitForFlushMemTable();
dbfull()->TEST_CompactRange(0, nullptr, nullptr);
ASSERT_EQ("2", FilesPerLevel());

dbfull()->TEST_CompactRange(0, nullptr, nullptr);
ASSERT_EQ("0,2", FilesPerLevel());
for (int idx = 0; idx < 22; ++idx) {
ASSERT_EQ(std::string(10000, 'a' + idx), Get(Key(idx)));
}
}

TEST(CuckooTableDBTest, CompactionIntoMultipleFiles) {
// Create a big L0 file and check it compacts into multiple files in L1.
Options options = CurrentOptions();
options.write_buffer_size = 270 << 10;
// Two SST files should be created, each containing 14 keys.
// Number of buckets will be 16. Total size ~156 KB.
options.target_file_size_base = 160 << 10;
Reopen(&options);

// Write 28 values, each 10016 B ~ 10KB
for (int idx = 0; idx < 28; ++idx) {
ASSERT_OK(Put(Key(idx), std::string(10000, 'a' + idx)));
}
dbfull()->TEST_WaitForFlushMemTable();
ASSERT_EQ("1", FilesPerLevel());

dbfull()->TEST_CompactRange(0, nullptr, nullptr);
ASSERT_EQ("0,2", FilesPerLevel());
for (int idx = 0; idx < 28; ++idx) {
ASSERT_EQ(std::string(10000, 'a' + idx), Get(Key(idx)));
}
}

TEST(CuckooTableDBTest, SameKeyInsertedInTwoDifferentFilesAndCompacted) {
// Insert same key twice so that they go to different SST files. Then wait for
// compaction and check if the latest value is stored and old value removed.
Expand Down
5 changes: 2 additions & 3 deletions table/cuckoo_table_builder.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,15 +56,14 @@ CuckooTableBuilder::CuckooTableBuilder(
ucomp_(user_comparator),
get_slice_hash_(get_slice_hash),
closed_(false) {
properties_.num_entries = 0;
// Data is in a huge block.
properties_.num_data_blocks = 1;
properties_.index_size = 0;
properties_.filter_size = 0;
}

void CuckooTableBuilder::Add(const Slice& key, const Slice& value) {
if (properties_.num_entries >= kMaxVectorIdx - 1) {
if (kvs_.size() >= kMaxVectorIdx - 1) {
status_ = Status::NotSupported("Number of keys in a file must be < 2^32-1");
return;
}
Expand Down Expand Up @@ -311,7 +310,7 @@ uint64_t CuckooTableBuilder::NumEntries() const {
uint64_t CuckooTableBuilder::FileSize() const {
if (closed_) {
return file_->GetFileSize();
} else if (properties_.num_entries == 0) {
} else if (kvs_.size() == 0) {
return 0;
}

Expand Down
41 changes: 33 additions & 8 deletions table/cuckoo_table_builder_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ TEST(CuckooBuilderTest, SuccessWithEmptyFile) {
CuckooTableBuilder builder(writable_file.get(), kHashTableRatio,
4, 100, BytewiseComparator(), 1, GetSliceHash);
ASSERT_OK(builder.status());
ASSERT_EQ(0UL, builder.FileSize());
ASSERT_OK(builder.Finish());
ASSERT_OK(writable_file->Close());
CheckFileContents({}, {}, {}, "", 0, 2, false);
Expand All @@ -155,6 +156,7 @@ TEST(CuckooBuilderTest, WriteSuccessNoCollisionFullKey) {
for (auto& user_key : user_keys) {
keys.push_back(GetInternalKey(user_key, false));
}
uint32_t expected_table_size = NextPowOf2(keys.size() / kHashTableRatio);

unique_ptr<WritableFile> writable_file;
fname = test::TmpDir() + "/NoCollisionFullKey";
Expand All @@ -167,10 +169,12 @@ TEST(CuckooBuilderTest, WriteSuccessNoCollisionFullKey) {
ASSERT_EQ(builder.NumEntries(), i + 1);
ASSERT_OK(builder.status());
}
uint32_t bucket_size = keys[0].size() + values[0].size();
ASSERT_LE(expected_table_size * bucket_size, builder.FileSize());
ASSERT_OK(builder.Finish());
ASSERT_OK(writable_file->Close());
ASSERT_LE(expected_table_size * bucket_size, builder.FileSize());

uint32_t expected_table_size = NextPowOf2(keys.size() / kHashTableRatio);
std::string expected_unused_bucket = GetInternalKey("key00", true);
expected_unused_bucket += std::string(values[0].size(), 'a');
CheckFileContents(keys, values, expected_locations,
Expand All @@ -192,6 +196,7 @@ TEST(CuckooBuilderTest, WriteSuccessWithCollisionFullKey) {
for (auto& user_key : user_keys) {
keys.push_back(GetInternalKey(user_key, false));
}
uint32_t expected_table_size = NextPowOf2(keys.size() / kHashTableRatio);

unique_ptr<WritableFile> writable_file;
fname = test::TmpDir() + "/WithCollisionFullKey";
Expand All @@ -204,10 +209,12 @@ TEST(CuckooBuilderTest, WriteSuccessWithCollisionFullKey) {
ASSERT_EQ(builder.NumEntries(), i + 1);
ASSERT_OK(builder.status());
}
uint32_t bucket_size = keys[0].size() + values[0].size();
ASSERT_LE(expected_table_size * bucket_size, builder.FileSize());
ASSERT_OK(builder.Finish());
ASSERT_OK(writable_file->Close());
ASSERT_LE(expected_table_size * bucket_size, builder.FileSize());

uint32_t expected_table_size = NextPowOf2(keys.size() / kHashTableRatio);
std::string expected_unused_bucket = GetInternalKey("key00", true);
expected_unused_bucket += std::string(values[0].size(), 'a');
CheckFileContents(keys, values, expected_locations,
Expand All @@ -229,6 +236,7 @@ TEST(CuckooBuilderTest, WriteSuccessWithCollisionAndCuckooBlock) {
for (auto& user_key : user_keys) {
keys.push_back(GetInternalKey(user_key, false));
}
uint32_t expected_table_size = NextPowOf2(keys.size() / kHashTableRatio);

unique_ptr<WritableFile> writable_file;
uint32_t cuckoo_block_size = 2;
Expand All @@ -242,10 +250,12 @@ TEST(CuckooBuilderTest, WriteSuccessWithCollisionAndCuckooBlock) {
ASSERT_EQ(builder.NumEntries(), i + 1);
ASSERT_OK(builder.status());
}
uint32_t bucket_size = keys[0].size() + values[0].size();
ASSERT_LE(expected_table_size * bucket_size, builder.FileSize());
ASSERT_OK(builder.Finish());
ASSERT_OK(writable_file->Close());
ASSERT_LE(expected_table_size * bucket_size, builder.FileSize());

uint32_t expected_table_size = NextPowOf2(keys.size() / kHashTableRatio);
std::string expected_unused_bucket = GetInternalKey("key00", true);
expected_unused_bucket += std::string(values[0].size(), 'a');
CheckFileContents(keys, values, expected_locations,
Expand All @@ -272,6 +282,7 @@ TEST(CuckooBuilderTest, WithCollisionPathFullKey) {
for (auto& user_key : user_keys) {
keys.push_back(GetInternalKey(user_key, false));
}
uint32_t expected_table_size = NextPowOf2(keys.size() / kHashTableRatio);

unique_ptr<WritableFile> writable_file;
fname = test::TmpDir() + "/WithCollisionPathFullKey";
Expand All @@ -284,10 +295,12 @@ TEST(CuckooBuilderTest, WithCollisionPathFullKey) {
ASSERT_EQ(builder.NumEntries(), i + 1);
ASSERT_OK(builder.status());
}
uint32_t bucket_size = keys[0].size() + values[0].size();
ASSERT_LE(expected_table_size * bucket_size, builder.FileSize());
ASSERT_OK(builder.Finish());
ASSERT_OK(writable_file->Close());
ASSERT_LE(expected_table_size * bucket_size, builder.FileSize());

uint32_t expected_table_size = NextPowOf2(keys.size() / kHashTableRatio);
std::string expected_unused_bucket = GetInternalKey("key00", true);
expected_unused_bucket += std::string(values[0].size(), 'a');
CheckFileContents(keys, values, expected_locations,
Expand All @@ -311,6 +324,7 @@ TEST(CuckooBuilderTest, WithCollisionPathFullKeyAndCuckooBlock) {
for (auto& user_key : user_keys) {
keys.push_back(GetInternalKey(user_key, false));
}
uint32_t expected_table_size = NextPowOf2(keys.size() / kHashTableRatio);

unique_ptr<WritableFile> writable_file;
fname = test::TmpDir() + "/WithCollisionPathFullKeyAndCuckooBlock";
Expand All @@ -323,10 +337,12 @@ TEST(CuckooBuilderTest, WithCollisionPathFullKeyAndCuckooBlock) {
ASSERT_EQ(builder.NumEntries(), i + 1);
ASSERT_OK(builder.status());
}
uint32_t bucket_size = keys[0].size() + values[0].size();
ASSERT_LE(expected_table_size * bucket_size, builder.FileSize());
ASSERT_OK(builder.Finish());
ASSERT_OK(writable_file->Close());
ASSERT_LE(expected_table_size * bucket_size, builder.FileSize());

uint32_t expected_table_size = NextPowOf2(keys.size() / kHashTableRatio);
std::string expected_unused_bucket = GetInternalKey("key00", true);
expected_unused_bucket += std::string(values[0].size(), 'a');
CheckFileContents(keys, values, expected_locations,
Expand All @@ -344,6 +360,7 @@ TEST(CuckooBuilderTest, WriteSuccessNoCollisionUserKey) {
{user_keys[3], {3, 4, 5, 6}}
};
std::vector<uint64_t> expected_locations = {0, 1, 2, 3};
uint32_t expected_table_size = NextPowOf2(user_keys.size() / kHashTableRatio);

unique_ptr<WritableFile> writable_file;
fname = test::TmpDir() + "/NoCollisionUserKey";
Expand All @@ -356,10 +373,12 @@ TEST(CuckooBuilderTest, WriteSuccessNoCollisionUserKey) {
ASSERT_EQ(builder.NumEntries(), i + 1);
ASSERT_OK(builder.status());
}
uint32_t bucket_size = user_keys[0].size() + values[0].size();
ASSERT_LE(expected_table_size * bucket_size, builder.FileSize());
ASSERT_OK(builder.Finish());
ASSERT_OK(writable_file->Close());
ASSERT_LE(expected_table_size * bucket_size, builder.FileSize());

uint32_t expected_table_size = NextPowOf2(user_keys.size() / kHashTableRatio);
std::string expected_unused_bucket = "key00";
expected_unused_bucket += std::string(values[0].size(), 'a');
CheckFileContents(user_keys, values, expected_locations,
Expand All @@ -377,6 +396,7 @@ TEST(CuckooBuilderTest, WriteSuccessWithCollisionUserKey) {
{user_keys[3], {0, 1, 2, 3}},
};
std::vector<uint64_t> expected_locations = {0, 1, 2, 3};
uint32_t expected_table_size = NextPowOf2(user_keys.size() / kHashTableRatio);

unique_ptr<WritableFile> writable_file;
fname = test::TmpDir() + "/WithCollisionUserKey";
Expand All @@ -389,10 +409,12 @@ TEST(CuckooBuilderTest, WriteSuccessWithCollisionUserKey) {
ASSERT_EQ(builder.NumEntries(), i + 1);
ASSERT_OK(builder.status());
}
uint32_t bucket_size = user_keys[0].size() + values[0].size();
ASSERT_LE(expected_table_size * bucket_size, builder.FileSize());
ASSERT_OK(builder.Finish());
ASSERT_OK(writable_file->Close());
ASSERT_LE(expected_table_size * bucket_size, builder.FileSize());

uint32_t expected_table_size = NextPowOf2(user_keys.size() / kHashTableRatio);
std::string expected_unused_bucket = "key00";
expected_unused_bucket += std::string(values[0].size(), 'a');
CheckFileContents(user_keys, values, expected_locations,
Expand All @@ -412,6 +434,7 @@ TEST(CuckooBuilderTest, WithCollisionPathUserKey) {
{user_keys[4], {0, 2}},
};
std::vector<uint64_t> expected_locations = {0, 1, 3, 4, 2};
uint32_t expected_table_size = NextPowOf2(user_keys.size() / kHashTableRatio);

unique_ptr<WritableFile> writable_file;
fname = test::TmpDir() + "/WithCollisionPathUserKey";
Expand All @@ -424,10 +447,12 @@ TEST(CuckooBuilderTest, WithCollisionPathUserKey) {
ASSERT_EQ(builder.NumEntries(), i + 1);
ASSERT_OK(builder.status());
}
uint32_t bucket_size = user_keys[0].size() + values[0].size();
ASSERT_LE(expected_table_size * bucket_size, builder.FileSize());
ASSERT_OK(builder.Finish());
ASSERT_OK(writable_file->Close());
ASSERT_LE(expected_table_size * bucket_size, builder.FileSize());

uint32_t expected_table_size = NextPowOf2(user_keys.size() / kHashTableRatio);
std::string expected_unused_bucket = "key00";
expected_unused_bucket += std::string(values[0].size(), 'a');
CheckFileContents(user_keys, values, expected_locations,
Expand Down

0 comments on commit 5cd0576

Please sign in to comment.