Skip to content

Commit

Permalink
Improve Options sanitization and add MmapReadRequired() to TableFactory
Browse files Browse the repository at this point in the history
Summary:
Currently, PlainTable must use mmap_reads.  When PlainTable is used but
allow_mmap_reads is not set, rocksdb will fail in flush.

This diff improve Options sanitization and add MmapReadRequired() to
TableFactory.

Test Plan:
export ROCKSDB_TESTS=PlainTableOptionsSanitizeTest
make db_test -j32
./db_test

Reviewers: sdong, ljin

Reviewed By: ljin

Subscribers: you, leveldb

Differential Revision: https://reviews.facebook.net/D21939
  • Loading branch information
yhchiang committed Aug 20, 2014
1 parent e173bf9 commit 63a2215
Show file tree
Hide file tree
Showing 7 changed files with 65 additions and 1 deletion.
19 changes: 18 additions & 1 deletion db/db_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,19 @@ DBOptions SanitizeOptions(const std::string& dbname, const DBOptions& src) {
return result;
}

Status SanitizeDBOptionsByCFOptions(
DBOptions* db_opts,
const std::vector<ColumnFamilyDescriptor>& column_families) {
Status s;
for (auto cf : column_families) {
s = cf.options.table_factory->SanitizeDBOptions(db_opts);
if (!s.ok()) {
return s;
}
}
return Status::OK();
}

namespace {
CompressionType GetCompressionFlush(const Options& options) {
// Compressing memtable flushes might not help unless the sequential load
Expand Down Expand Up @@ -4743,7 +4756,11 @@ Status DB::Open(const Options& options, const std::string& dbname, DB** dbptr) {
column_families.push_back(
ColumnFamilyDescriptor(kDefaultColumnFamilyName, cf_options));
std::vector<ColumnFamilyHandle*> handles;
Status s = DB::Open(db_options, dbname, column_families, &handles, dbptr);
Status s = SanitizeDBOptionsByCFOptions(&db_options, column_families);
if (!s.ok()) {
return s;
}
s = DB::Open(db_options, dbname, column_families, &handles, dbptr);
if (s.ok()) {
assert(handles.size() == 1);
// i can delete the handle since DBImpl is always holding a reference to
Expand Down
12 changes: 12 additions & 0 deletions db/db_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7679,6 +7679,18 @@ TEST(DBTest, RateLimitingTest) {
ASSERT_TRUE(ratio < 0.6);
}

TEST(DBTest, TableOptionsSanitizeTest) {
Options options = CurrentOptions();
options.create_if_missing = true;
DestroyAndReopen(&options);
ASSERT_EQ(db_->GetOptions().allow_mmap_reads, false);

options.table_factory.reset(new PlainTableFactory());
options.prefix_extractor.reset(NewNoopTransform());
Destroy(&options);
ASSERT_TRUE(TryReopen(&options).IsNotSupported());
}

} // namespace rocksdb

int main(int argc, char** argv) {
Expand Down
6 changes: 6 additions & 0 deletions include/rocksdb/table.h
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,12 @@ class TableFactory {
virtual TableBuilder* NewTableBuilder(
const Options& options, const InternalKeyComparator& internal_comparator,
WritableFile* file, CompressionType compression_type) const = 0;

// Sanitizes the specified DB Options.
//
// If the function cannot find a way to sanitize the input DB Options,
// a non-ok Status will be returned.
virtual Status SanitizeDBOptions(DBOptions* db_opts) const = 0;
};

#ifndef ROCKSDB_LITE
Expand Down
10 changes: 10 additions & 0 deletions table/adaptive_table_factory.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,16 @@ class AdaptiveTableFactory : public TableFactory {
CompressionType compression_type) const
override;

// Sanitizes the specified DB Options.
Status SanitizeDBOptions(DBOptions* db_opts) const override {
if (db_opts->allow_mmap_reads == false) {
return Status::NotSupported(
"AdaptiveTable with allow_mmap_reads == false is not supported.");
}
return Status::OK();
}


private:
std::shared_ptr<TableFactory> table_factory_to_write_;
std::shared_ptr<TableFactory> block_based_table_factory_;
Expand Down
5 changes: 5 additions & 0 deletions table/block_based_table_factory.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,11 @@ class BlockBasedTableFactory : public TableFactory {
const Options& options, const InternalKeyComparator& internal_comparator,
WritableFile* file, CompressionType compression_type) const override;

// Sanitizes the specified DB Options.
Status SanitizeDBOptions(DBOptions* db_opts) const override {
return Status::OK();
}

private:
BlockBasedTableOptions table_options_;
};
Expand Down
5 changes: 5 additions & 0 deletions table/cuckoo_table_factory.h
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,11 @@ class CuckooTableFactory : public TableFactory {
const InternalKeyComparator& icomparator, WritableFile* file,
CompressionType compression_type) const override;

// Sanitizes the specified DB Options.
Status SanitizeDBOptions(DBOptions* db_opts) const override {
return Status::OK();
}

private:
const double hash_table_ratio_;
const uint32_t max_search_depth_;
Expand Down
9 changes: 9 additions & 0 deletions table/plain_table_factory.h
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,15 @@ class PlainTableFactory : public TableFactory {

static const char kValueTypeSeqId0 = 0xFF;

// Sanitizes the specified DB Options.
Status SanitizeDBOptions(DBOptions* db_opts) const override {
if (db_opts->allow_mmap_reads == false) {
return Status::NotSupported(
"PlainTable with allow_mmap_reads == false is not supported.");
}
return Status::OK();
}

private:
uint32_t user_key_len_;
int bloom_bits_per_key_;
Expand Down

0 comments on commit 63a2215

Please sign in to comment.