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

Compute NeedCompact() after table builder Finish() (#7627)

Browse Source 
Summary:
In `BuildTable()`, we call `builder->Finish()` before evaluating `builder->NeedCompact()`.
However, we call `builder->NeedCompact()` before `builder->Finish()` in compaction job. This can be wrong because the table properties collectors may rely on the success of `Finish()` to provide correct result for `NeedCompact()`.

Test plan (on devserver):
make check

Pull Request resolved: https://github.com/facebook/rocksdb/pull/7627

Reviewed By: ajkr

Differential Revision: D24728741

Pulled By: riversand963

fbshipit-source-id: 5a0dce244e14eb1106c4f87021e6bebca82b486e
This commit is contained in:
Yanqin Jin 2020-11-04 10:43:17 -08:00
parent d4b82746b2
commit d5673571bf
2 changed files with 49 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
db/compaction/compaction_job.cc
View File

@ -1352,7 +1352,6 @@ Status CompactionJob::FinishCompactionOutputFile(
ExtractInternalKeyFooter(meta->smallest.Encode()) != ExtractInternalKeyFooter(meta->smallest.Encode()) !=
PackSequenceAndType(0, kTypeRangeDeletion)); PackSequenceAndType(0, kTypeRangeDeletion));
} }
meta->marked_for_compaction = sub_compact->builder->NeedCompact();
} }
const uint64_t current_entries = sub_compact->builder->NumEntries(); const uint64_t current_entries = sub_compact->builder->NumEntries();
if (s.ok()) { if (s.ok()) {
@ -1367,6 +1366,7 @@ Status CompactionJob::FinishCompactionOutputFile(
const uint64_t current_bytes = sub_compact->builder->FileSize(); const uint64_t current_bytes = sub_compact->builder->FileSize();
if (s.ok()) { if (s.ok()) {
meta->fd.file_size = current_bytes; meta->fd.file_size = current_bytes;
meta->marked_for_compaction = sub_compact->builder->NeedCompact();
} }
sub_compact->current_output()->finished = true; sub_compact->current_output()->finished = true;
sub_compact->total_bytes += current_bytes; sub_compact->total_bytes += current_bytes;

48
db/db_table_properties_test.cc
View File

@ -370,6 +370,54 @@ TEST_P(DBTablePropertiesTest, DeletionTriggeredCompactionMarking) {
ASSERT_LT(0, opts.statistics->getTickerCount(COMPACT_READ_BYTES_MARKED)); ASSERT_LT(0, opts.statistics->getTickerCount(COMPACT_READ_BYTES_MARKED));
} }
TEST_P(DBTablePropertiesTest, RatioBasedDeletionTriggeredCompactionMarking) {
constexpr int kNumKeys = 1000;
constexpr int kWindowSize = 0;
constexpr int kNumDelsTrigger = 0;
constexpr double kDeletionRatio = 0.1;
std::shared_ptr<TablePropertiesCollectorFactory> compact_on_del =
NewCompactOnDeletionCollectorFactory(kWindowSize, kNumDelsTrigger,
kDeletionRatio);
Options opts = CurrentOptions();
opts.statistics = ROCKSDB_NAMESPACE::CreateDBStatistics();
opts.table_properties_collector_factories.emplace_back(compact_on_del);
Reopen(opts);
// Add an L2 file to prevent tombstones from dropping due to obsolescence
// during flush
Put(Key(0), "val");
Flush();
MoveFilesToLevel(2);
auto* listener = new DeletionTriggeredCompactionTestListener();
opts.listeners.emplace_back(listener);
Reopen(opts);
// Generate one L0 with kNumKeys Put.
for (int i = 0; i < kNumKeys; ++i) {
ASSERT_OK(Put(Key(i), "not important"));
}
ASSERT_OK(Flush());
// Generate another L0 with kNumKeys Delete.
// This file, due to deletion ratio, will trigger compaction: 2@0 files to L1.
// The resulting L1 file has only one tombstone for user key 'Key(0)'.
// Again, due to deletion ratio, a compaction will be triggered: 1@1 + 1@2
// files to L2. However, the resulting file is empty because the tombstone
// and value are both dropped.
for (int i = 0; i < kNumKeys; ++i) {
ASSERT_OK(Delete(Key(i)));
}
ASSERT_OK(Flush());
ASSERT_OK(dbfull()->TEST_WaitForCompact());
for (int i = 0; i < 3; ++i) {
ASSERT_EQ(0, NumTableFilesAtLevel(i));
}
}
INSTANTIATE_TEST_CASE_P( INSTANTIATE_TEST_CASE_P(
DBTablePropertiesTest, DBTablePropertiesTest,
DBTablePropertiesTest, DBTablePropertiesTest,