From a8c89cc96983be8884bf93de484c13f76685bcf9 Mon Sep 17 00:00:00 2001 From: mrambacher Date: Wed, 14 Oct 2020 22:27:12 -0700 Subject: [PATCH] Test for LoadLatestOptions (#7554) Summary: Make LoadLatestOptions return PathNotFound if the options file does not exist. Added tests for the LoadOptions related methods. Pull Request resolved: https://github.com/facebook/rocksdb/pull/7554 Reviewed By: akankshamahajan15 Differential Revision: D24298985 Pulled By: zhichao-cao fbshipit-source-id: c9ae3cb12fc4a5bbef07743e1c1300f98a2441b3 --- db/db_impl/db_impl.cc | 1 + memtable/write_buffer_manager.cc | 6 +- options/options_parser.cc | 12 +- options/options_test.cc | 8 +- utilities/options/options_util.cc | 8 +- utilities/options/options_util_test.cc | 253 ++++++++++++++++++++++++- 6 files changed, 266 insertions(+), 22 deletions(-) diff --git a/db/db_impl/db_impl.cc b/db/db_impl/db_impl.cc index 0229e798a..50e8d711a 100644 --- a/db/db_impl/db_impl.cc +++ b/db/db_impl/db_impl.cc @@ -976,6 +976,7 @@ Status DBImpl::SetOptions( MutableCFOptions new_options; Status s; Status persist_options_status; + persist_options_status.PermitUncheckedError(); // Allow uninitialized access SuperVersionContext sv_context(/* create_superversion */ true); { auto db_options = GetDBOptions(); diff --git a/memtable/write_buffer_manager.cc b/memtable/write_buffer_manager.cc index 23ca1d749..9b7470870 100644 --- a/memtable/write_buffer_manager.cc +++ b/memtable/write_buffer_manager.cc @@ -91,8 +91,10 @@ void WriteBufferManager::ReserveMemWithCache(size_t mem) { // Expand size by at least 256KB. // Add a dummy record to the cache Cache::Handle* handle = nullptr; - cache_rep_->cache_->Insert(cache_rep_->GetNextCacheKey(), nullptr, - kSizeDummyEntry, nullptr, &handle); + Status s = + cache_rep_->cache_->Insert(cache_rep_->GetNextCacheKey(), nullptr, + kSizeDummyEntry, nullptr, &handle); + s.PermitUncheckedError(); // TODO: What to do on error? // We keep the handle even if insertion fails and a null handle is // returned, so that when memory shrinks, we don't release extra // entries from cache. diff --git a/options/options_parser.cc b/options/options_parser.cc index 4ce578b41..6642d4115 100644 --- a/options/options_parser.cc +++ b/options/options_parser.cc @@ -289,13 +289,13 @@ Status RocksDBOptionsParser::Parse(const ConfigOptions& config_options_in, return s; } - // If the option file is not generated by a higher minor version, - // there shouldn't be any unknown option. - if (config_options.ignore_unknown_options && + // If the option file is newer than the current version + // there may be unknown options. + if (!config_options.ignore_unknown_options && section == kOptionSectionVersion) { - if (db_version[0] < ROCKSDB_MAJOR || (db_version[0] == ROCKSDB_MAJOR && - db_version[1] <= ROCKSDB_MINOR)) { - config_options.ignore_unknown_options = false; + if (db_version[0] > ROCKSDB_MAJOR || + (db_version[0] == ROCKSDB_MAJOR && db_version[1] > ROCKSDB_MINOR)) { + config_options.ignore_unknown_options = true; } } diff --git a/options/options_test.cc b/options/options_test.cc index 510b9ee1e..ee81f3573 100644 --- a/options/options_test.cc +++ b/options/options_test.cc @@ -2633,15 +2633,15 @@ TEST_F(OptionsParserTest, IgnoreUnknownOptions) { } ASSERT_OK(env_->WriteToNewFile(kTestFileName, options_file_content)); RocksDBOptionsParser parser; - ASSERT_NOK(parser.Parse(kTestFileName, fs_.get(), false, - 4096 /* readahead_size */)); + ASSERT_OK(parser.Parse(kTestFileName, fs_.get(), true, + 4096 /* readahead_size */)); if (should_ignore) { ASSERT_OK(parser.Parse(kTestFileName, fs_.get(), - true /* ignore_unknown_options */, + false /* ignore_unknown_options */, 4096 /* readahead_size */)); } else { ASSERT_NOK(parser.Parse(kTestFileName, fs_.get(), - true /* ignore_unknown_options */, + false /* ignore_unknown_options */, 4096 /* readahead_size */)); } } diff --git a/utilities/options/options_util.cc b/utilities/options/options_util.cc index 2eb398ed5..3aa8625be 100644 --- a/utilities/options/options_util.cc +++ b/utilities/options/options_util.cc @@ -65,7 +65,10 @@ Status GetLatestOptionsFileName(const std::string& dbpath, uint64_t latest_time_stamp = 0; std::vector file_names; s = env->GetChildren(dbpath, &file_names); - if (!s.ok()) { + if (s.IsNotFound()) { + return Status::PathNotFound("No options files found in the DB directory.", + dbpath); + } else if (!s.ok()) { return s; } for (auto& file_name : file_names) { @@ -79,7 +82,8 @@ Status GetLatestOptionsFileName(const std::string& dbpath, } } if (latest_file_name.size() == 0) { - return Status::NotFound("No options files found in the DB directory."); + return Status::PathNotFound("No options files found in the DB directory.", + dbpath); } *options_file_name = latest_file_name; return Status::OK(); diff --git a/utilities/options/options_util_test.cc b/utilities/options/options_util_test.cc index 515eacc56..ccfef3471 100644 --- a/utilities/options/options_util_test.cc +++ b/utilities/options/options_util_test.cc @@ -11,6 +11,8 @@ #include #include +#include "env/mock_env.h" +#include "file/filename.h" #include "options/options_parser.h" #include "rocksdb/convenience.h" #include "rocksdb/db.h" @@ -31,14 +33,12 @@ namespace ROCKSDB_NAMESPACE { class OptionsUtilTest : public testing::Test { public: OptionsUtilTest() : rnd_(0xFB) { - env_.reset(new test::StringEnv(Env::Default())); - fs_.reset(new LegacyFileSystemWrapper(env_.get())); + env_.reset(NewMemEnv(Env::Default())); dbname_ = test::PerThreadDBPath("options_util_test"); } protected: - std::unique_ptr env_; - std::unique_ptr fs_; + std::unique_ptr env_; std::string dbname_; Random rnd_; }; @@ -58,8 +58,8 @@ TEST_F(OptionsUtilTest, SaveAndLoad) { } const std::string kFileName = "OPTIONS-123456"; - ASSERT_OK( - PersistRocksDBOptions(db_opt, cf_names, cf_opts, kFileName, fs_.get())); + ASSERT_OK(PersistRocksDBOptions(db_opt, cf_names, cf_opts, kFileName, + env_->GetFileSystem().get())); DBOptions loaded_db_opt; std::vector loaded_cf_descs; @@ -85,6 +85,7 @@ TEST_F(OptionsUtilTest, SaveAndLoad) { exact, cf_opts[i], loaded_cf_descs[i].options)); } + DestroyDB(dbname_, Options(db_opt, cf_opts[0])); for (size_t i = 0; i < kCFCount; ++i) { if (cf_opts[i].compaction_filter) { delete cf_opts[i].compaction_filter; @@ -121,8 +122,8 @@ TEST_F(OptionsUtilTest, SaveAndLoadWithCacheCheck) { cf_names.push_back("cf_plain_table_sample"); // Saving DB in file const std::string kFileName = "OPTIONS-LOAD_CACHE_123456"; - ASSERT_OK( - PersistRocksDBOptions(db_opt, cf_names, cf_opts, kFileName, fs_.get())); + ASSERT_OK(PersistRocksDBOptions(db_opt, cf_names, cf_opts, kFileName, + env_->GetFileSystem().get())); DBOptions loaded_db_opt; std::vector loaded_cf_descs; @@ -154,6 +155,7 @@ TEST_F(OptionsUtilTest, SaveAndLoadWithCacheCheck) { ASSERT_EQ(loaded_bbt_opt->block_cache.get(), cache.get()); } } + DestroyDB(dbname_, Options(loaded_db_opt, cf_opts[0])); } namespace { @@ -359,8 +361,243 @@ TEST_F(OptionsUtilTest, SanityCheck) { ASSERT_OK( CheckOptionsCompatibility(config_options, dbname_, db_opt, cf_descs)); } + DestroyDB(dbname_, Options(db_opt, cf_descs[0].options)); +} + +TEST_F(OptionsUtilTest, LatestOptionsNotFound) { + std::unique_ptr env(NewMemEnv(Env::Default())); + Status s; + Options options; + ConfigOptions config_opts; + std::vector cf_descs; + + options.env = env.get(); + options.create_if_missing = true; + config_opts.env = options.env; + config_opts.ignore_unknown_options = false; + + std::vector children; + + std::string options_file_name; + DestroyDB(dbname_, options); + // First, test where the db directory does not exist + ASSERT_NOK(options.env->GetChildren(dbname_, &children)); + + s = GetLatestOptionsFileName(dbname_, options.env, &options_file_name); + ASSERT_TRUE(s.IsPathNotFound()); + + s = LoadLatestOptions(dbname_, options.env, &options, &cf_descs); + ASSERT_TRUE(s.IsPathNotFound()); + + s = LoadLatestOptions(config_opts, dbname_, &options, &cf_descs); + ASSERT_TRUE(s.IsPathNotFound()); + + s = GetLatestOptionsFileName(dbname_, options.env, &options_file_name); + ASSERT_TRUE(s.IsPathNotFound()); + + // Second, test where the db directory exists but is empty + ASSERT_OK(options.env->CreateDir(dbname_)); + + s = GetLatestOptionsFileName(dbname_, options.env, &options_file_name); + ASSERT_TRUE(s.IsPathNotFound()); + + s = LoadLatestOptions(dbname_, options.env, &options, &cf_descs); + ASSERT_TRUE(s.IsPathNotFound()); + + // Finally, test where a file exists but is not an "Options" file + std::unique_ptr file; + ASSERT_OK( + options.env->NewWritableFile(dbname_ + "/temp.txt", &file, EnvOptions())); + ASSERT_OK(file->Close()); + s = GetLatestOptionsFileName(dbname_, options.env, &options_file_name); + ASSERT_TRUE(s.IsPathNotFound()); + + s = LoadLatestOptions(config_opts, dbname_, &options, &cf_descs); + ASSERT_TRUE(s.IsPathNotFound()); + ASSERT_OK(options.env->DeleteFile(dbname_ + "/temp.txt")); + ASSERT_OK(options.env->DeleteDir(dbname_)); +} + +TEST_F(OptionsUtilTest, LoadLatestOptions) { + Options options; + options.OptimizeForSmallDb(); + ColumnFamilyDescriptor cf_desc; + ConfigOptions config_opts; + DBOptions db_opts; + std::vector cf_descs; + std::vector handles; + DB* db; + options.create_if_missing = true; + + DestroyDB(dbname_, options); + + cf_descs.emplace_back(); + cf_descs.back().name = kDefaultColumnFamilyName; + cf_descs.back().options.table_factory.reset(NewBlockBasedTableFactory()); + cf_descs.emplace_back(); + cf_descs.back().name = "Plain"; + cf_descs.back().options.table_factory.reset(NewPlainTableFactory()); + db_opts.create_missing_column_families = true; + db_opts.create_if_missing = true; + + // open and persist the options + ASSERT_OK(DB::Open(db_opts, dbname_, cf_descs, &handles, &db)); + + std::string options_file_name; + std::string new_options_file; + + ASSERT_OK(GetLatestOptionsFileName(dbname_, options.env, &options_file_name)); + ASSERT_OK(LoadLatestOptions(config_opts, dbname_, &db_opts, &cf_descs)); + ASSERT_EQ(cf_descs.size(), 2U); + ASSERT_OK(RocksDBOptionsParser::VerifyDBOptions(config_opts, + db->GetDBOptions(), db_opts)); + ASSERT_OK(handles[0]->GetDescriptor(&cf_desc)); + ASSERT_OK(RocksDBOptionsParser::VerifyCFOptions(config_opts, cf_desc.options, + cf_descs[0].options)); + ASSERT_OK(handles[1]->GetDescriptor(&cf_desc)); + ASSERT_OK(RocksDBOptionsParser::VerifyCFOptions(config_opts, cf_desc.options, + cf_descs[1].options)); + + // Now change some of the DBOptions + ASSERT_OK(db->SetDBOptions( + {{"delayed_write_rate", "1234"}, {"bytes_per_sync", "32768"}})); + ASSERT_OK(GetLatestOptionsFileName(dbname_, options.env, &new_options_file)); + ASSERT_NE(options_file_name, new_options_file); + ASSERT_OK(LoadLatestOptions(config_opts, dbname_, &db_opts, &cf_descs)); + ASSERT_OK(RocksDBOptionsParser::VerifyDBOptions(config_opts, + db->GetDBOptions(), db_opts)); + options_file_name = new_options_file; + + // Now change some of the ColumnFamilyOptions + ASSERT_OK(db->SetOptions(handles[1], {{"write_buffer_size", "32768"}})); + ASSERT_OK(GetLatestOptionsFileName(dbname_, options.env, &new_options_file)); + ASSERT_NE(options_file_name, new_options_file); + ASSERT_OK(LoadLatestOptions(config_opts, dbname_, &db_opts, &cf_descs)); + ASSERT_OK(RocksDBOptionsParser::VerifyDBOptions(config_opts, + db->GetDBOptions(), db_opts)); + ASSERT_OK(handles[0]->GetDescriptor(&cf_desc)); + ASSERT_OK(RocksDBOptionsParser::VerifyCFOptions(config_opts, cf_desc.options, + cf_descs[0].options)); + ASSERT_OK(handles[1]->GetDescriptor(&cf_desc)); + ASSERT_OK(RocksDBOptionsParser::VerifyCFOptions(config_opts, cf_desc.options, + cf_descs[1].options)); + + // close the db + for (auto* handle : handles) { + delete handle; + } + delete db; + DestroyDB(dbname_, options, cf_descs); } +static void WriteOptionsFile(Env* env, const std::string& path, + const std::string& options_file, int major, + int minor, const std::string& db_opts, + const std::string& cf_opts) { + std::string options_file_header = + "\n" + "[Version]\n" + " rocksdb_version=" + + ToString(major) + "." + ToString(minor) + + ".0\n" + " options_file_version=1\n"; + + std::unique_ptr wf; + ASSERT_OK(env->NewWritableFile(path + "/" + options_file, &wf, EnvOptions())); + ASSERT_OK( + wf->Append(options_file_header + "[ DBOptions ]\n" + db_opts + "\n")); + ASSERT_OK(wf->Append( + "[CFOptions \"default\"] # column family must be specified\n" + + cf_opts + "\n")); + ASSERT_OK(wf->Close()); + + std::string latest_options_file; + ASSERT_OK(GetLatestOptionsFileName(path, env, &latest_options_file)); + ASSERT_EQ(latest_options_file, options_file); +} + +TEST_F(OptionsUtilTest, BadLatestOptions) { + Status s; + ConfigOptions config_opts; + DBOptions db_opts; + std::vector cf_descs; + Options options; + options.env = env_.get(); + config_opts.env = env_.get(); + config_opts.ignore_unknown_options = false; + config_opts.delimiter = "\n"; + + ConfigOptions ignore_opts = config_opts; + ignore_opts.ignore_unknown_options = true; + + std::string options_file_name; + + // Test where the db directory exists but is empty + ASSERT_OK(options.env->CreateDir(dbname_)); + ASSERT_NOK( + GetLatestOptionsFileName(dbname_, options.env, &options_file_name)); + ASSERT_NOK(LoadLatestOptions(config_opts, dbname_, &db_opts, &cf_descs)); + + // Write an options file for a previous major release with an unknown DB + // Option + WriteOptionsFile(options.env, dbname_, "OPTIONS-0001", ROCKSDB_MAJOR - 1, + ROCKSDB_MINOR, "unknown_db_opt=true", ""); + s = LoadLatestOptions(config_opts, dbname_, &db_opts, &cf_descs); + ASSERT_NOK(s); + ASSERT_TRUE(s.IsNotFound()); + ASSERT_OK(LoadLatestOptions(ignore_opts, dbname_, &db_opts, &cf_descs)); + + // Write an options file for a previous minor release with an unknown CF + // Option + WriteOptionsFile(options.env, dbname_, "OPTIONS-0002", ROCKSDB_MAJOR, + ROCKSDB_MINOR - 1, "", "unknown_cf_opt=true"); + s = LoadLatestOptions(config_opts, dbname_, &db_opts, &cf_descs); + ASSERT_NOK(s); + ASSERT_TRUE(s.IsNotFound()); + ASSERT_OK(LoadLatestOptions(ignore_opts, dbname_, &db_opts, &cf_descs)); + + // Write an options file for the current release with an unknown DB Option + WriteOptionsFile(options.env, dbname_, "OPTIONS-0003", ROCKSDB_MAJOR, + ROCKSDB_MINOR, "unknown_db_opt=true", ""); + s = LoadLatestOptions(config_opts, dbname_, &db_opts, &cf_descs); + ASSERT_NOK(s); + ASSERT_TRUE(s.IsNotFound()); + ASSERT_OK(LoadLatestOptions(ignore_opts, dbname_, &db_opts, &cf_descs)); + + // Write an options file for the current release with an unknown CF Option + WriteOptionsFile(options.env, dbname_, "OPTIONS-0004", ROCKSDB_MAJOR, + ROCKSDB_MINOR, "", "unknown_cf_opt=true"); + s = LoadLatestOptions(config_opts, dbname_, &db_opts, &cf_descs); + ASSERT_NOK(s); + ASSERT_TRUE(s.IsNotFound()); + ASSERT_OK(LoadLatestOptions(ignore_opts, dbname_, &db_opts, &cf_descs)); + + // Write an options file for the current release with an invalid DB Option + WriteOptionsFile(options.env, dbname_, "OPTIONS-0005", ROCKSDB_MAJOR, + ROCKSDB_MINOR, "create_if_missing=hello", ""); + s = LoadLatestOptions(config_opts, dbname_, &db_opts, &cf_descs); + ASSERT_NOK(s); + ASSERT_TRUE(s.IsInvalidArgument()); + ASSERT_OK(LoadLatestOptions(ignore_opts, dbname_, &db_opts, &cf_descs)); + + // Write an options file for the next release with an invalid DB Option + WriteOptionsFile(options.env, dbname_, "OPTIONS-0006", ROCKSDB_MAJOR, + ROCKSDB_MINOR + 1, "create_if_missing=hello", ""); + ASSERT_OK(LoadLatestOptions(config_opts, dbname_, &db_opts, &cf_descs)); + ASSERT_OK(LoadLatestOptions(ignore_opts, dbname_, &db_opts, &cf_descs)); + + // Write an options file for the next release with an unknown DB Option + WriteOptionsFile(options.env, dbname_, "OPTIONS-0007", ROCKSDB_MAJOR, + ROCKSDB_MINOR + 1, "unknown_db_opt=true", ""); + ASSERT_OK(LoadLatestOptions(config_opts, dbname_, &db_opts, &cf_descs)); + ASSERT_OK(LoadLatestOptions(ignore_opts, dbname_, &db_opts, &cf_descs)); + + // Write an options file for the next major release with an unknown CF Option + WriteOptionsFile(options.env, dbname_, "OPTIONS-0008", ROCKSDB_MAJOR + 1, + ROCKSDB_MINOR, "", "unknown_cf_opt=true"); + ASSERT_OK(LoadLatestOptions(config_opts, dbname_, &db_opts, &cf_descs)); + ASSERT_OK(LoadLatestOptions(ignore_opts, dbname_, &db_opts, &cf_descs)); +} } // namespace ROCKSDB_NAMESPACE int main(int argc, char** argv) {