Skip to content

Commit

Permalink
Unified maps with Comparator for sorting, other cleanup
Browse files Browse the repository at this point in the history 
Summary:
This diff is a collection of cleanups that were initially part of D43179.
Additionally it adds a unified way of defining key-value maps that use a
Comparator for sorting (this was previously implemented in four different
places).

Test Plan: make clean check all

Reviewers: rven, anthony, yhchiang, sdong, igor

Reviewed By: igor

Subscribers: dhruba, leveldb

Differential Revision: https://reviews.facebook.net/D45993
  • Loading branch information
@4tXJ7f
4tXJ7f committed Sep 2, 2015
1 parent 3e0a672 commit 3c9cef1
Show file tree
Hide file tree
Showing 24 changed files with 289 additions and 324 deletions.
4 changes: 2 additions & 2 deletions db/builder.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ Status BuildTable(

// In-memory key corruption is not ok;
// TODO: find a clean way to treat in memory key corruption
// Ugly walkaround to avoid compiler error for release build
// Ugly workaround to avoid compiler error for release build
bool ok __attribute__((unused)) = true;
ok = ParseInternalKey(key, &ikey);
assert(ok);
Expand All @@ -159,7 +159,7 @@ Status BuildTable(

// If there are no snapshots, then this kv affect visibility at tip.
// Otherwise, search though all existing snapshots to find
// the earlist snapshot that is affected by this kv.
// the earliest snapshot that is affected by this kv.
SequenceNumber prev_snapshot = 0; // 0 means no previous snapshot
SequenceNumber key_needs_to_exist_in_snapshot =
EarliestVisibleSnapshot(ikey.sequence, snapshots, &prev_snapshot);
Expand Down
24 changes: 12 additions & 12 deletions db/compaction_job.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,35 +10,35 @@

#include <atomic>
#include <deque>
#include <functional>
#include <limits>
#include <set>
#include <string>
#include <utility>
#include <vector>
#include <string>
#include <functional>

#include "db/column_family.h"
#include "db/dbformat.h"
#include "db/flush_scheduler.h"
#include "db/internal_stats.h"
#include "db/job_context.h"
#include "db/log_writer.h"
#include "db/column_family.h"
#include "db/version_edit.h"
#include "db/memtable_list.h"
#include "db/version_edit.h"
#include "db/write_controller.h"
#include "db/write_thread.h"
#include "port/port.h"
#include "rocksdb/compaction_filter.h"
#include "rocksdb/compaction_job_stats.h"
#include "rocksdb/db.h"
#include "rocksdb/env.h"
#include "rocksdb/memtablerep.h"
#include "rocksdb/compaction_filter.h"
#include "rocksdb/compaction_job_stats.h"
#include "rocksdb/transaction_log.h"
#include "util/autovector.h"
#include "util/event_logger.h"
#include "util/scoped_arena_iterator.h"
#include "util/stop_watch.h"
#include "util/thread_local.h"
#include "util/scoped_arena_iterator.h"
#include "db/internal_stats.h"
#include "db/write_controller.h"
#include "db/flush_scheduler.h"
#include "db/write_thread.h"
#include "db/job_context.h"

namespace rocksdb {

Expand Down
123 changes: 52 additions & 71 deletions db/compaction_job_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ class CompactionJobTest : public testing::Test {
return InternalKey(user_key, seq_num, t).Encode().ToString();
}

void AddMockFile(const mock::MockFileContents& contents, int level = 0) {
void AddMockFile(const stl_wrappers::KVMap& contents, int level = 0) {
assert(contents.size() > 0);

bool first_key = true;
Expand Down Expand Up @@ -143,8 +143,8 @@ class CompactionJobTest : public testing::Test {
}

// returns expected result after compaction
mock::MockFileContents CreateTwoFiles(bool gen_corrupted_keys) {
mock::MockFileContents expected_results;
stl_wrappers::KVMap CreateTwoFiles(bool gen_corrupted_keys) {
auto expected_results = mock::MakeMockFile();
const int kKeysPerFile = 10000;
const int kCorruptKeysPerFile = 200;
const int kMatchingKeys = kKeysPerFile / 2;
Expand All @@ -155,7 +155,7 @@ class CompactionJobTest : public testing::Test {
};

for (int i = 0; i < 2; ++i) {
mock::MockFileContents contents;
auto contents = mock::MakeMockFile();
for (int k = 0; k < kKeysPerFile; ++k) {
auto key = ToString(i * kMatchingKeys + k);
auto value = ToString(i * kKeysPerFile + k);
Expand Down Expand Up @@ -215,7 +215,7 @@ class CompactionJobTest : public testing::Test {
}

void RunCompaction(const std::vector<std::vector<FileMetaData*>>& input_files,
const mock::MockFileContents& expected_results) {
const stl_wrappers::KVMap& expected_results) {
auto cfd = versions_->GetColumnFamilySet()->GetDefault();

size_t num_input_files = 0;
Expand Down Expand Up @@ -303,21 +303,16 @@ TEST_F(CompactionJobTest, SimpleCorrupted) {
TEST_F(CompactionJobTest, SimpleDeletion) {
NewDB();

mock::MockFileContents file1 = {
{ KeyStr("c", 4U, kTypeDeletion), "" },
{ KeyStr("c", 3U, kTypeValue), "val" }
};
auto file1 = mock::MakeMockFile({{KeyStr("c", 4U, kTypeDeletion), ""},
{KeyStr("c", 3U, kTypeValue), "val"}});
AddMockFile(file1);

mock::MockFileContents file2 = {
{ KeyStr("b", 2U, kTypeValue), "val" },
{ KeyStr("b", 1U, kTypeValue), "val" }
};
auto file2 = mock::MakeMockFile({{KeyStr("b", 2U, kTypeValue), "val"},
{KeyStr("b", 1U, kTypeValue), "val"}});
AddMockFile(file2);

mock::MockFileContents expected_results = {
{ KeyStr("b", 0U, kTypeValue), "val" }
};
auto expected_results =
mock::MakeMockFile({{KeyStr("b", 0U, kTypeValue), "val"}});

SetLastSequence(4U);
auto files = cfd_->current()->storage_info()->LevelFiles(0);
Expand All @@ -327,22 +322,19 @@ TEST_F(CompactionJobTest, SimpleDeletion) {
TEST_F(CompactionJobTest, SimpleOverwrite) {
NewDB();

mock::MockFileContents file1 = {
{ KeyStr("a", 3U, kTypeValue), "val2" },
{ KeyStr("b", 4U, kTypeValue), "val3" },
};
auto file1 = mock::MakeMockFile({
{KeyStr("a", 3U, kTypeValue), "val2"},
{KeyStr("b", 4U, kTypeValue), "val3"},
});
AddMockFile(file1);

mock::MockFileContents file2 = {
{ KeyStr("a", 1U, kTypeValue), "val" },
{ KeyStr("b", 2U, kTypeValue), "val" }
};
auto file2 = mock::MakeMockFile({{KeyStr("a", 1U, kTypeValue), "val"},
{KeyStr("b", 2U, kTypeValue), "val"}});
AddMockFile(file2);

mock::MockFileContents expected_results = {
{ KeyStr("a", 0U, kTypeValue), "val2" },
{ KeyStr("b", 0U, kTypeValue), "val3" }
};
auto expected_results =
mock::MakeMockFile({{KeyStr("a", 0U, kTypeValue), "val2"},
{KeyStr("b", 0U, kTypeValue), "val3"}});

SetLastSequence(4U);
auto files = cfd_->current()->storage_info()->LevelFiles(0);
Expand All @@ -352,30 +344,25 @@ TEST_F(CompactionJobTest, SimpleOverwrite) {
TEST_F(CompactionJobTest, SimpleNonLastLevel) {
NewDB();

mock::MockFileContents file1 = {
{ KeyStr("a", 5U, kTypeValue), "val2" },
{ KeyStr("b", 6U, kTypeValue), "val3" },
};
auto file1 = mock::MakeMockFile({
{KeyStr("a", 5U, kTypeValue), "val2"},
{KeyStr("b", 6U, kTypeValue), "val3"},
});
AddMockFile(file1);

mock::MockFileContents file2 = {
{ KeyStr("a", 3U, kTypeValue), "val" },
{ KeyStr("b", 4U, kTypeValue), "val" }
};
auto file2 = mock::MakeMockFile({{KeyStr("a", 3U, kTypeValue), "val"},
{KeyStr("b", 4U, kTypeValue), "val"}});
AddMockFile(file2, 1);

mock::MockFileContents file3 = {
{ KeyStr("a", 1U, kTypeValue), "val" },
{ KeyStr("b", 2U, kTypeValue), "val" }
};
auto file3 = mock::MakeMockFile({{KeyStr("a", 1U, kTypeValue), "val"},
{KeyStr("b", 2U, kTypeValue), "val"}});
AddMockFile(file3, 2);

// Because level 1 is not the last level, the sequence numbers of a and b
// cannot be set to 0
mock::MockFileContents expected_results = {
{ KeyStr("a", 5U, kTypeValue), "val2" },
{ KeyStr("b", 6U, kTypeValue), "val3" }
};
auto expected_results =
mock::MakeMockFile({{KeyStr("a", 5U, kTypeValue), "val2"},
{KeyStr("b", 6U, kTypeValue), "val3"}});

SetLastSequence(6U);
auto lvl0_files = cfd_->current()->storage_info()->LevelFiles(0);
Expand All @@ -387,23 +374,20 @@ TEST_F(CompactionJobTest, SimpleMerge) {
auto merge_op = MergeOperators::CreateStringAppendOperator();
NewDB(merge_op);

mock::MockFileContents file1 = {
{ KeyStr("a", 5U, kTypeMerge), "5" },
{ KeyStr("a", 4U, kTypeMerge), "4" },
{ KeyStr("a", 3U, kTypeValue), "3" },
};
auto file1 = mock::MakeMockFile({
{KeyStr("a", 5U, kTypeMerge), "5"},
{KeyStr("a", 4U, kTypeMerge), "4"},
{KeyStr("a", 3U, kTypeValue), "3"},
});
AddMockFile(file1);

mock::MockFileContents file2 = {
{ KeyStr("b", 2U, kTypeMerge), "2" },
{ KeyStr("b", 1U, kTypeValue), "1" }
};
auto file2 = mock::MakeMockFile(
{{KeyStr("b", 2U, kTypeMerge), "2"}, {KeyStr("b", 1U, kTypeValue), "1"}});
AddMockFile(file2);

mock::MockFileContents expected_results = {
{ KeyStr("a", 0U, kTypeValue), "3,4,5" },
{ KeyStr("b", 0U, kTypeValue), "1,2" }
};
auto expected_results =
mock::MakeMockFile({{KeyStr("a", 0U, kTypeValue), "3,4,5"},
{KeyStr("b", 0U, kTypeValue), "1,2"}});

SetLastSequence(5U);
auto files = cfd_->current()->storage_info()->LevelFiles(0);
Expand All @@ -414,24 +398,21 @@ TEST_F(CompactionJobTest, NonAssocMerge) {
auto merge_op = MergeOperators::CreateStringAppendTESTOperator();
NewDB(merge_op);

mock::MockFileContents file1 = {
{ KeyStr("a", 5U, kTypeMerge), "5" },
{ KeyStr("a", 4U, kTypeMerge), "4" },
{ KeyStr("a", 3U, kTypeMerge), "3" },
};
auto file1 = mock::MakeMockFile({
{KeyStr("a", 5U, kTypeMerge), "5"},
{KeyStr("a", 4U, kTypeMerge), "4"},
{KeyStr("a", 3U, kTypeMerge), "3"},
});
AddMockFile(file1);

mock::MockFileContents file2 = {
{ KeyStr("b", 2U, kTypeMerge), "2" },
{ KeyStr("b", 1U, kTypeMerge), "1" }
};
auto file2 = mock::MakeMockFile(
{{KeyStr("b", 2U, kTypeMerge), "2"}, {KeyStr("b", 1U, kTypeMerge), "1"}});
AddMockFile(file2);

mock::MockFileContents expected_results = {
{ KeyStr("a", 0U, kTypeValue), "3,4,5" },
{ KeyStr("b", 2U, kTypeMerge), "2" },
{ KeyStr("b", 1U, kTypeMerge), "1" }
};
auto expected_results =
mock::MakeMockFile({{KeyStr("a", 0U, kTypeValue), "3,4,5"},
{KeyStr("b", 2U, kTypeMerge), "2"},
{KeyStr("b", 1U, kTypeMerge), "1"}});

SetLastSequence(5U);
auto files = cfd_->current()->storage_info()->LevelFiles(0);
Expand Down
19 changes: 6 additions & 13 deletions db/comparator_db_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "rocksdb/db.h"
#include "rocksdb/env.h"
#include "util/hash.h"
#include "util/stl_wrappers.h"
#include "util/string_util.h"
#include "util/testharness.h"
#include "util/testutil.h"
Expand All @@ -22,18 +23,10 @@ namespace {

static const Comparator* comparator;

// A comparator for std::map, using comparator
struct MapComparator {
bool operator()(const std::string& a, const std::string& b) const {
return comparator->Compare(a, b) < 0;
}
};

typedef std::map<std::string, std::string, MapComparator> KVMap;

class KVIter : public Iterator {
public:
explicit KVIter(const KVMap* map) : map_(map), iter_(map_->end()) {}
explicit KVIter(const stl_wrappers::KVMap* map)
: map_(map), iter_(map_->end()) {}
virtual bool Valid() const override { return iter_ != map_->end(); }
virtual void SeekToFirst() override { iter_ = map_->begin(); }
virtual void SeekToLast() override {
Expand All @@ -60,8 +53,8 @@ class KVIter : public Iterator {
virtual Status status() const override { return Status::OK(); }

private:
const KVMap* const map_;
KVMap::const_iterator iter_;
const stl_wrappers::KVMap* const map_;
stl_wrappers::KVMap::const_iterator iter_;
};

void AssertItersEqual(Iterator* iter1, Iterator* iter2) {
Expand All @@ -77,7 +70,7 @@ void AssertItersEqual(Iterator* iter1, Iterator* iter2) {
void DoRandomIteraratorTest(DB* db, std::vector<std::string> source_strings,
Random* rnd, int num_writes, int num_iter_ops,
int num_trigger_flush) {
KVMap map;
stl_wrappers::KVMap map((stl_wrappers::LessOfComparator(comparator)));

for (int i = 0; i < num_writes; i++) {
if (num_trigger_flush > 0 && i != 0 && i % num_trigger_flush == 0) {
Expand Down
Loading

0 comments on commit 3c9cef1

Please sign in to comment.