From cde47c7b856a7e503f0f2c6ab266c0df67a1cfd5 Mon Sep 17 00:00:00 2001 From: myabandeh Date: Wed, 29 May 2019 10:00:59 -0700 Subject: [PATCH] call ValidateOptions from SetDBOptions remove redudant BuildDBOptions remove mistaken OptimizeForCompactionTableRead on write options ValidateOptions in SetOptions make format return correct status exclude invalids from random options minor Disable setting snappy option in Appveyor exclude unsupported compression types from random gen put back OptimizeForCompactionTableRead --- db/column_family.cc | 49 ++++++++++++++++++- db/column_family.h | 4 ++ db/db_impl/db_impl.cc | 29 ++++++++--- db/db_impl/db_impl.h | 7 +++ db/db_impl/db_impl_open.cc | 46 +++-------------- db/db_options_test.cc | 4 +- db/db_test.cc | 3 ++ options/options_test.cc | 7 +-- test_util/testutil.cc | 15 ++++-- test_util/testutil.h | 2 +- utilities/options/options_util_test.cc | 4 +- .../transactions/write_prepared_txn_db.cc | 2 +- 12 files changed, 114 insertions(+), 58 deletions(-) diff --git a/db/column_family.cc b/db/column_family.cc index ce22a00aac3..531cbeca681 100644 --- a/db/column_family.cc +++ b/db/column_family.cc @@ -1148,13 +1148,60 @@ void ColumnFamilyData::ResetThreadLocalSuperVersions() { } } +Status ColumnFamilyData::ValidateOptions( + const DBOptions& db_options, const ColumnFamilyOptions& cf_options) { + Status s; + s = CheckCompressionSupported(cf_options); + if (s.ok() && db_options.allow_concurrent_memtable_write) { + s = CheckConcurrentWritesSupported(cf_options); + } + if (s.ok()) { + s = CheckCFPathsSupported(db_options, cf_options); + } + if (!s.ok()) { + return s; + } + + if (cf_options.ttl > 0) { + if (db_options.max_open_files != -1) { + return Status::NotSupported( + "TTL is only supported when files are always " + "kept open (set max_open_files = -1). "); + } + if (cf_options.table_factory->Name() != BlockBasedTableFactory().Name()) { + return Status::NotSupported( + "TTL is only supported in Block-Based Table format. "); + } + } + + if (cf_options.periodic_compaction_seconds > 0) { + if (db_options.max_open_files != -1) { + return Status::NotSupported( + "Periodic Compaction is only supported when files are always " + "kept open (set max_open_files = -1). "); + } + if (cf_options.table_factory->Name() != BlockBasedTableFactory().Name()) { + return Status::NotSupported( + "Periodic Compaction is only supported in " + "Block-Based Table format. "); + } + } + return s; +} + #ifndef ROCKSDB_LITE Status ColumnFamilyData::SetOptions( - const std::unordered_map& options_map) { + const DBOptions& db_options, + const std::unordered_map& options_map) { MutableCFOptions new_mutable_cf_options; Status s = GetMutableOptionsFromStrings(mutable_cf_options_, options_map, ioptions_.info_log, &new_mutable_cf_options); + if (s.ok()) { + ColumnFamilyOptions cf_options = + BuildColumnFamilyOptions(initial_cf_options_, new_mutable_cf_options); + s = ValidateOptions(db_options, cf_options); + } if (s.ok()) { mutable_cf_options_ = new_mutable_cf_options; mutable_cf_options_.RefreshDerivedOptions(ioptions_); diff --git a/db/column_family.h b/db/column_family.h index 655cb159261..8646b4fc197 100644 --- a/db/column_family.h +++ b/db/column_family.h @@ -338,9 +338,13 @@ class ColumnFamilyData { bool is_delete_range_supported() { return is_delete_range_supported_; } + // Validate CF options against DB options + static Status ValidateOptions(const DBOptions& db_options, + const ColumnFamilyOptions& cf_options); #ifndef ROCKSDB_LITE // REQUIRES: DB mutex held Status SetOptions( + const DBOptions& db_options, const std::unordered_map& options_map); #endif // ROCKSDB_LITE diff --git a/db/db_impl/db_impl.cc b/db/db_impl/db_impl.cc index 196e38f14fa..435dbbfe6bf 100644 --- a/db/db_impl/db_impl.cc +++ b/db/db_impl/db_impl.cc @@ -848,8 +848,9 @@ Status DBImpl::SetOptions( Status persist_options_status; SuperVersionContext sv_context(/* create_superversion */ true); { + auto db_options = GetDBOptions(); InstrumentedMutexLock l(&mutex_); - s = cfd->SetOptions(options_map); + s = cfd->SetOptions(db_options, options_map); if (s.ok()) { new_options = *cfd->GetLatestMutableCFOptions(); // Append new version to recompute compaction score. @@ -912,6 +913,25 @@ Status DBImpl::SetDBOptions( InstrumentedMutexLock l(&mutex_); s = GetMutableDBOptionsFromStrings(mutable_db_options_, options_map, &new_options); + if (new_options.bytes_per_sync == 0) { + new_options.bytes_per_sync = 1024 * 1024; + } + DBOptions new_db_options = + BuildDBOptions(immutable_db_options_, new_options); + if (s.ok()) { + s = ValidateOptions(new_db_options); + } + if (s.ok()) { + for (auto c : *versions_->GetColumnFamilySet()) { + if (!c->IsDropped()) { + auto cf_options = c->GetLatestCFOptions(); + s = ColumnFamilyData::ValidateOptions(new_db_options, cf_options); + if (!s.ok()) { + break; + } + } + } + } if (s.ok()) { if (new_options.max_background_compactions > mutable_db_options_.max_background_compactions) { @@ -956,15 +976,12 @@ Status DBImpl::SetDBOptions( : new_options.max_open_files - 10); wal_changed = mutable_db_options_.wal_bytes_per_sync != new_options.wal_bytes_per_sync; - if (new_options.bytes_per_sync == 0) { - new_options.bytes_per_sync = 1024 * 1024; - } mutable_db_options_ = new_options; - env_options_for_compaction_ = EnvOptions( - BuildDBOptions(immutable_db_options_, mutable_db_options_)); + env_options_for_compaction_ = EnvOptions(new_db_options); env_options_for_compaction_ = env_->OptimizeForCompactionTableWrite( env_options_for_compaction_, immutable_db_options_); versions_->ChangeEnvOptions(mutable_db_options_); + //TODO(xiez): clarify why apply optimize for read to write options env_options_for_compaction_ = env_->OptimizeForCompactionTableRead( env_options_for_compaction_, immutable_db_options_); env_options_for_compaction_.compaction_readahead_size = diff --git a/db/db_impl/db_impl.h b/db/db_impl/db_impl.h index c241a36dbc3..1f8f990e4f7 100644 --- a/db/db_impl/db_impl.h +++ b/db/db_impl/db_impl.h @@ -1500,6 +1500,13 @@ class DBImpl : public DB { Status CreateWAL(uint64_t log_file_num, uint64_t recycle_log_number, size_t preallocate_block_size, log::Writer** new_log); + // Validate self-consistency of DB options + static Status ValidateOptions(const DBOptions& db_options); + // Validate self-consistency of DB options and its consistency with cf options + static Status ValidateOptions( + const DBOptions& db_options, + const std::vector& column_families); + // table_cache_ provides its own synchronization std::shared_ptr table_cache_; diff --git a/db/db_impl/db_impl_open.cc b/db/db_impl/db_impl_open.cc index 5019221b5ca..2fc12746d7d 100644 --- a/db/db_impl/db_impl_open.cc +++ b/db/db_impl/db_impl_open.cc @@ -145,7 +145,6 @@ DBOptions SanitizeOptions(const std::string& dbname, const DBOptions& src) { } namespace { - Status SanitizeOptionsByTable( const DBOptions& db_opts, const std::vector& column_families) { @@ -158,52 +157,23 @@ Status SanitizeOptionsByTable( } return Status::OK(); } +} // namespace -static Status ValidateOptions( +Status DBImpl::ValidateOptions( const DBOptions& db_options, const std::vector& column_families) { Status s; - for (auto& cfd : column_families) { - s = CheckCompressionSupported(cfd.options); - if (s.ok() && db_options.allow_concurrent_memtable_write) { - s = CheckConcurrentWritesSupported(cfd.options); - } - if (s.ok()) { - s = CheckCFPathsSupported(db_options, cfd.options); - } + s = ColumnFamilyData::ValidateOptions(db_options, cfd.options); if (!s.ok()) { return s; } - - if (cfd.options.ttl > 0) { - if (db_options.max_open_files != -1) { - return Status::NotSupported( - "TTL is only supported when files are always " - "kept open (set max_open_files = -1). "); - } - if (cfd.options.table_factory->Name() != - BlockBasedTableFactory().Name()) { - return Status::NotSupported( - "TTL is only supported in Block-Based Table format. "); - } - } - - if (cfd.options.periodic_compaction_seconds > 0) { - if (db_options.max_open_files != -1) { - return Status::NotSupported( - "Periodic Compaction is only supported when files are always " - "kept open (set max_open_files = -1). "); - } - if (cfd.options.table_factory->Name() != - BlockBasedTableFactory().Name()) { - return Status::NotSupported( - "Periodic Compaction is only supported in " - "Block-Based Table format. "); - } - } } + s = ValidateOptions(db_options); + return s; +} +Status DBImpl::ValidateOptions(const DBOptions& db_options) { if (db_options.db_paths.size() > 4) { return Status::NotSupported( "More than four DB paths are not supported yet. "); @@ -241,7 +211,7 @@ static Status ValidateOptions( return Status::OK(); } -} // namespace + Status DBImpl::NewDB() { VersionEdit new_db; new_db.SetLogNumber(0); diff --git a/db/db_options_test.cc b/db/db_options_test.cc index a9c8d218235..bf33153284e 100644 --- a/db/db_options_test.cc +++ b/db/db_options_test.cc @@ -66,10 +66,10 @@ class DBOptionsTest : public DBTestBase { std::unordered_map GetRandomizedMutableCFOptionsMap( Random* rnd) { - Options options; + Options options = CurrentOptions(); options.env = env_; ImmutableDBOptions db_options(options); - test::RandomInitCFOptions(&options, rnd); + test::RandomInitCFOptions(&options, options, rnd); auto sanitized_options = SanitizeOptions(db_options, options); auto opt_map = GetMutableCFOptionsMap(sanitized_options); delete options.compaction_filter; diff --git a/db/db_test.cc b/db/db_test.cc index 4c4bd382ca8..27cf790ee57 100644 --- a/db/db_test.cc +++ b/db/db_test.cc @@ -4884,11 +4884,14 @@ TEST_F(DBTest, DynamicMiscOptions) { ASSERT_OK(dbfull()->TEST_GetLatestMutableCFOptions(handles_[0], &mutable_cf_options)); ASSERT_EQ(CompressionType::kNoCompression, mutable_cf_options.compression); + // Appveyor fails with: Compression type Snappy is not linked with the binary +#ifndef OS_WIN ASSERT_OK(dbfull()->SetOptions({{"compression", "kSnappyCompression"}})); ASSERT_OK(dbfull()->TEST_GetLatestMutableCFOptions(handles_[0], &mutable_cf_options)); ASSERT_EQ(CompressionType::kSnappyCompression, mutable_cf_options.compression); +#endif // Test paranoid_file_checks already done in db_block_cache_test ASSERT_OK( dbfull()->SetOptions(handles_[1], {{"paranoid_file_checks", "true"}})); diff --git a/options/options_test.cc b/options/options_test.cc index 429b607e4f9..1aa3bace7dd 100644 --- a/options/options_test.cc +++ b/options/options_test.cc @@ -842,7 +842,7 @@ TEST_F(OptionsTest, OptionsComposeDecompose) { Random rnd(301); test::RandomInitDBOptions(&base_db_opts, &rnd); - test::RandomInitCFOptions(&base_cf_opts, &rnd); + test::RandomInitCFOptions(&base_cf_opts, base_db_opts, &rnd); Options base_opts(base_db_opts, base_cf_opts); DBOptions new_db_opts(base_opts); @@ -854,11 +854,12 @@ TEST_F(OptionsTest, OptionsComposeDecompose) { } TEST_F(OptionsTest, ColumnFamilyOptionsSerialization) { + Options options; ColumnFamilyOptions base_opt, new_opt; Random rnd(302); // Phase 1: randomly assign base_opt // custom type options - test::RandomInitCFOptions(&base_opt, &rnd); + test::RandomInitCFOptions(&base_opt, options, &rnd); // Phase 2: obtain a string from base_opt std::string base_options_file_content; @@ -1521,7 +1522,7 @@ TEST_F(OptionsParserTest, DumpAndParse) { for (int c = 0; c < num_cf; ++c) { ColumnFamilyOptions cf_opt; Random cf_rnd(0xFB + c); - test::RandomInitCFOptions(&cf_opt, &cf_rnd); + test::RandomInitCFOptions(&cf_opt, base_db_opt, &cf_rnd); if (c < 4) { cf_opt.prefix_extractor.reset(test::RandomSliceTransform(&rnd, c)); } diff --git a/test_util/testutil.cc b/test_util/testutil.cc index 18e1a45bb36..4e37cde40d1 100644 --- a/test_util/testutil.cc +++ b/test_util/testutil.cc @@ -162,7 +162,11 @@ std::string RandomName(Random* rnd, const size_t len) { } CompressionType RandomCompressionType(Random* rnd) { - return static_cast(rnd->Uniform(6)); + auto ret = static_cast(rnd->Uniform(6)); + while (!CompressionTypeSupported(ret)) { + ret = static_cast((static_cast(ret) + 1) % 6); + } + return ret; } void RandomCompressionTypeVector(const size_t count, @@ -293,7 +297,8 @@ void RandomInitDBOptions(DBOptions* db_opt, Random* rnd) { db_opt->stats_dump_period_sec = rnd->Uniform(100000); } -void RandomInitCFOptions(ColumnFamilyOptions* cf_opt, Random* rnd) { +void RandomInitCFOptions(ColumnFamilyOptions* cf_opt, DBOptions& db_options, + Random* rnd) { cf_opt->compaction_style = (CompactionStyle)(rnd->Uniform(4)); // boolean options @@ -345,8 +350,10 @@ void RandomInitCFOptions(ColumnFamilyOptions* cf_opt, Random* rnd) { // uint64_t options static const uint64_t uint_max = static_cast(UINT_MAX); - cf_opt->ttl = uint_max + rnd->Uniform(10000); - cf_opt->periodic_compaction_seconds = uint_max + rnd->Uniform(10000); + cf_opt->ttl = + db_options.max_open_files == -1 ? uint_max + rnd->Uniform(10000) : 0; + cf_opt->periodic_compaction_seconds = + db_options.max_open_files == -1 ? uint_max + rnd->Uniform(10000) : 0; cf_opt->max_sequential_skip_in_iterations = uint_max + rnd->Uniform(10000); cf_opt->target_file_size_base = uint_max + rnd->Uniform(10000); cf_opt->max_compaction_bytes = diff --git a/test_util/testutil.h b/test_util/testutil.h index 7890ce5f511..bc0b2b07d5f 100644 --- a/test_util/testutil.h +++ b/test_util/testutil.h @@ -657,7 +657,7 @@ void RandomInitDBOptions(DBOptions* db_opt, Random* rnd); // Randomly initialize the given ColumnFamilyOptions // Note that the caller is responsible for releasing non-null // cf_opt->compaction_filter. -void RandomInitCFOptions(ColumnFamilyOptions* cf_opt, Random* rnd); +void RandomInitCFOptions(ColumnFamilyOptions* cf_opt, DBOptions&, Random* rnd); // A dummy merge operator which can change its name class ChanglingMergeOperator : public MergeOperator { diff --git a/utilities/options/options_util_test.cc b/utilities/options/options_util_test.cc index 5b8015152ff..8c71dbf5dc3 100644 --- a/utilities/options/options_util_test.cc +++ b/utilities/options/options_util_test.cc @@ -58,7 +58,7 @@ TEST_F(OptionsUtilTest, SaveAndLoad) { cf_names.push_back(i == 0 ? kDefaultColumnFamilyName : test::RandomName(&rnd_, 10)); cf_opts.emplace_back(); - test::RandomInitCFOptions(&cf_opts.back(), &rnd_); + test::RandomInitCFOptions(&cf_opts.back(), db_opt, &rnd_); } const std::string kFileName = "OPTIONS-123456"; @@ -82,7 +82,7 @@ TEST_F(OptionsUtilTest, SaveAndLoad) { cf_opts[i].table_factory.get(), loaded_cf_descs[i].options.table_factory.get())); } - test::RandomInitCFOptions(&cf_opts[i], &rnd_); + test::RandomInitCFOptions(&cf_opts[i], db_opt, &rnd_); ASSERT_NOK(RocksDBOptionsParser::VerifyCFOptions( cf_opts[i], loaded_cf_descs[i].options)); } diff --git a/utilities/transactions/write_prepared_txn_db.cc b/utilities/transactions/write_prepared_txn_db.cc index bf94d83d82b..e2a8fbbf20f 100644 --- a/utilities/transactions/write_prepared_txn_db.cc +++ b/utilities/transactions/write_prepared_txn_db.cc @@ -210,7 +210,7 @@ Status WritePreparedTxnDB::WriteInternal(const WriteOptions& write_options_orig, WriteBatch empty_batch; write_options.disableWAL = true; write_options.sync = false; - const size_t ONE_BATCH = 1; // Just to inc the seq + const size_t ONE_BATCH = 1; // Just to inc the seq s = db_impl_->WriteImpl(write_options, &empty_batch, nullptr, nullptr, no_log_ref, DISABLE_MEMTABLE, &seq_used, ONE_BATCH, &update_commit_map_with_prepare);