From 1eda625eab491402efaa2cad17c5964008ad7d10 Mon Sep 17 00:00:00 2001 From: mrambacher Date: Tue, 20 Oct 2020 11:51:51 -0700 Subject: [PATCH] Revert `Status`es returned from pre-`Configurable` options functions (#7563) Summary: Further refinement of the earlier PR. Now the Status is NotFound with a subcode of PathNotFound. Also the existing functions for options parsing/loading are reverted to return InvalidArgument no matter in which way the user-provided arguments are deemed invalid. Pull Request resolved: https://github.com/facebook/rocksdb/pull/7563 Reviewed By: zhichao-cao Differential Revision: D24422491 Pulled By: ajkr fbshipit-source-id: ba6b237cd0584d3f925c5ba0d349aeb8c250af67 --- include/rocksdb/status.h | 9 +- options/options_helper.cc | 31 +++-- options/options_test.cc | 106 ++++++++++++------ .../block_based/block_based_table_factory.cc | 10 +- table/plain/plain_table_factory.cc | 10 +- utilities/options/options_util.cc | 10 +- utilities/options/options_util_test.cc | 15 ++- 7 files changed, 134 insertions(+), 57 deletions(-) diff --git a/include/rocksdb/status.h b/include/rocksdb/status.h index dc76c7ce7..bcc55e4fd 100644 --- a/include/rocksdb/status.h +++ b/include/rocksdb/status.h @@ -162,9 +162,15 @@ class Status { static Status NotFound(const Slice& msg, const Slice& msg2 = Slice()) { return Status(kNotFound, msg, msg2); } + // Fast path for not found without malloc; static Status NotFound(SubCode msg = kNone) { return Status(kNotFound, msg); } + static Status NotFound(SubCode sc, const Slice& msg, + const Slice& msg2 = Slice()) { + return Status(kNotFound, sc, msg, msg2); + } + static Status Corruption(const Slice& msg, const Slice& msg2 = Slice()) { return Status(kCorruption, msg, msg2); } @@ -463,7 +469,8 @@ class Status { #ifdef ROCKSDB_ASSERT_STATUS_CHECKED checked_ = true; #endif // ROCKSDB_ASSERT_STATUS_CHECKED - return (code() == kIOError) && (subcode() == kPathNotFound); + return (code() == kIOError || code() == kNotFound) && + (subcode() == kPathNotFound); } // Returns true iff the status indicates manual compaction paused. This diff --git a/options/options_helper.cc b/options/options_helper.cc index a30006d1c..e0f8d26cd 100644 --- a/options/options_helper.cc +++ b/options/options_helper.cc @@ -727,9 +727,15 @@ Status GetColumnFamilyOptionsFromMap( *new_options = base_options; const auto config = CFOptionsAsConfigurable(base_options); - return ConfigureFromMap(config_options, opts_map, - OptionsHelper::kCFOptionsName, - config.get(), new_options); + Status s = ConfigureFromMap( + config_options, opts_map, OptionsHelper::kCFOptionsName, config.get(), + new_options); + // Translate any errors (NotFound, NotSupported, to InvalidArgument + if (s.ok() || s.IsInvalidArgument()) { + return s; + } else { + return Status::InvalidArgument(s.getState()); + } } Status GetColumnFamilyOptionsFromString( @@ -776,9 +782,15 @@ Status GetDBOptionsFromMap( assert(new_options); *new_options = base_options; auto config = DBOptionsAsConfigurable(base_options); - return ConfigureFromMap(config_options, opts_map, - OptionsHelper::kDBOptionsName, - config.get(), new_options); + Status s = ConfigureFromMap(config_options, opts_map, + OptionsHelper::kDBOptionsName, + config.get(), new_options); + // Translate any errors (NotFound, NotSupported, to InvalidArgument + if (s.ok() || s.IsInvalidArgument()) { + return s; + } else { + return Status::InvalidArgument(s.getState()); + } } Status GetDBOptionsFromString(const DBOptions& base_options, @@ -844,7 +856,12 @@ Status GetOptionsFromString(const ConfigOptions& config_options, *new_options = Options(*new_db_options, base_options); } } - return s; + // Translate any errors (NotFound, NotSupported, to InvalidArgument + if (s.ok() || s.IsInvalidArgument()) { + return s; + } else { + return Status::InvalidArgument(s.getState()); + } } std::unordered_map diff --git a/options/options_test.cc b/options/options_test.cc index ee81f3573..f568208aa 100644 --- a/options/options_test.cc +++ b/options/options_test.cc @@ -304,8 +304,11 @@ TEST_F(OptionsTest, GetOptionsFromMapTest) { ASSERT_EQ(new_db_opt.strict_bytes_per_sync, true); db_options_map["max_open_files"] = "hello"; - ASSERT_NOK( - GetDBOptionsFromMap(exact, base_db_opt, db_options_map, &new_db_opt)); + Status s = + GetDBOptionsFromMap(exact, base_db_opt, db_options_map, &new_db_opt); + ASSERT_NOK(s); + ASSERT_TRUE(s.IsInvalidArgument()); + ASSERT_OK( RocksDBOptionsParser::VerifyDBOptions(exact, base_db_opt, new_db_opt)); ASSERT_OK( @@ -313,8 +316,9 @@ TEST_F(OptionsTest, GetOptionsFromMapTest) { // unknow options should fail parsing without ignore_unknown_options = true db_options_map["unknown_db_option"] = "1"; - ASSERT_NOK( - GetDBOptionsFromMap(exact, base_db_opt, db_options_map, &new_db_opt)); + s = GetDBOptionsFromMap(exact, base_db_opt, db_options_map, &new_db_opt); + ASSERT_NOK(s); + ASSERT_TRUE(s.IsInvalidArgument()); ASSERT_OK( RocksDBOptionsParser::VerifyDBOptions(exact, base_db_opt, new_db_opt)); @@ -399,22 +403,29 @@ TEST_F(OptionsTest, GetColumnFamilyOptionsFromStringTest) { ASSERT_EQ(kMoName, std::string(new_cf_opt.merge_operator->Name())); // Wrong key/value pair - ASSERT_NOK(GetColumnFamilyOptionsFromString( + Status s = GetColumnFamilyOptionsFromString( config_options, base_cf_opt, - "write_buffer_size=13;max_write_buffer_number;", &new_cf_opt)); + "write_buffer_size=13;max_write_buffer_number;", &new_cf_opt); + ASSERT_NOK(s); + ASSERT_TRUE(s.IsInvalidArgument()); + ASSERT_OK(RocksDBOptionsParser::VerifyCFOptions(config_options, base_cf_opt, new_cf_opt)); - // Error Paring value - ASSERT_NOK(GetColumnFamilyOptionsFromString( + // Error Parsing value + s = GetColumnFamilyOptionsFromString( config_options, base_cf_opt, - "write_buffer_size=13;max_write_buffer_number=;", &new_cf_opt)); + "write_buffer_size=13;max_write_buffer_number=;", &new_cf_opt); + ASSERT_NOK(s); + ASSERT_TRUE(s.IsInvalidArgument()); ASSERT_OK(RocksDBOptionsParser::VerifyCFOptions(config_options, base_cf_opt, new_cf_opt)); // Missing option name - ASSERT_NOK(GetColumnFamilyOptionsFromString( - config_options, base_cf_opt, "write_buffer_size=13; =100;", &new_cf_opt)); + s = GetColumnFamilyOptionsFromString( + config_options, base_cf_opt, "write_buffer_size=13; =100;", &new_cf_opt); + ASSERT_NOK(s); + ASSERT_TRUE(s.IsInvalidArgument()); ASSERT_OK(RocksDBOptionsParser::VerifyCFOptions(config_options, base_cf_opt, new_cf_opt)); @@ -787,7 +798,10 @@ TEST_F(OptionsTest, OldInterfaceTest) { ASSERT_EQ(new_db_opt.track_and_verify_wals_in_manifest, true); ASSERT_EQ(new_db_opt.max_open_files, 32); db_options_map["unknown_option"] = "1"; - ASSERT_NOK(GetDBOptionsFromMap(base_db_opt, db_options_map, &new_db_opt)); + Status s = GetDBOptionsFromMap(base_db_opt, db_options_map, &new_db_opt); + ASSERT_NOK(s); + ASSERT_TRUE(s.IsInvalidArgument()); + ASSERT_OK( RocksDBOptionsParser::VerifyDBOptions(exact, base_db_opt, new_db_opt)); ASSERT_OK(GetDBOptionsFromMap(base_db_opt, db_options_map, &new_db_opt, true, @@ -799,11 +813,13 @@ TEST_F(OptionsTest, OldInterfaceTest) { ASSERT_EQ(new_db_opt.create_if_missing, false); ASSERT_EQ(new_db_opt.error_if_exists, false); ASSERT_EQ(new_db_opt.max_open_files, 42); - ASSERT_NOK(GetDBOptionsFromString( + s = GetDBOptionsFromString( base_db_opt, "create_if_missing=false;error_if_exists=false;max_open_files=42;" "unknown_option=1;", - &new_db_opt)); + &new_db_opt); + ASSERT_NOK(s); + ASSERT_TRUE(s.IsInvalidArgument()); ASSERT_OK( RocksDBOptionsParser::VerifyDBOptions(exact, base_db_opt, new_db_opt)); } @@ -848,19 +864,23 @@ TEST_F(OptionsTest, GetBlockBasedTableOptionsFromString) { EXPECT_EQ(bfp.GetWholeBitsPerKey(), 5); // unknown option - ASSERT_NOK(GetBlockBasedTableOptionsFromString( + Status s = GetBlockBasedTableOptionsFromString( config_options, table_opt, "cache_index_and_filter_blocks=1;index_type=kBinarySearch;" "bad_option=1", - &new_opt)); + &new_opt); + ASSERT_NOK(s); + ASSERT_TRUE(s.IsInvalidArgument()); ASSERT_EQ(static_cast(table_opt.cache_index_and_filter_blocks), new_opt.cache_index_and_filter_blocks); ASSERT_EQ(table_opt.index_type, new_opt.index_type); // unrecognized index type - ASSERT_NOK(GetBlockBasedTableOptionsFromString( + s = GetBlockBasedTableOptionsFromString( config_options, table_opt, - "cache_index_and_filter_blocks=1;index_type=kBinarySearchXX", &new_opt)); + "cache_index_and_filter_blocks=1;index_type=kBinarySearchXX", &new_opt); + ASSERT_NOK(s); + ASSERT_TRUE(s.IsInvalidArgument()); ASSERT_EQ(table_opt.cache_index_and_filter_blocks, new_opt.cache_index_and_filter_blocks); ASSERT_EQ(table_opt.index_type, new_opt.index_type); @@ -874,21 +894,23 @@ TEST_F(OptionsTest, GetBlockBasedTableOptionsFromString) { ASSERT_EQ(table_opt.index_type, new_opt.index_type); // unrecognized filter policy name - ASSERT_NOK( - GetBlockBasedTableOptionsFromString(config_options, table_opt, + s = GetBlockBasedTableOptionsFromString(config_options, table_opt, "cache_index_and_filter_blocks=1;" "filter_policy=bloomfilterxx:4:true", - &new_opt)); + &new_opt); + ASSERT_NOK(s); + ASSERT_TRUE(s.IsInvalidArgument()); ASSERT_EQ(table_opt.cache_index_and_filter_blocks, new_opt.cache_index_and_filter_blocks); ASSERT_EQ(table_opt.filter_policy, new_opt.filter_policy); // unrecognized filter policy config - ASSERT_NOK( - GetBlockBasedTableOptionsFromString(config_options, table_opt, + s = GetBlockBasedTableOptionsFromString(config_options, table_opt, "cache_index_and_filter_blocks=1;" "filter_policy=bloomfilter:4", - &new_opt)); + &new_opt); + ASSERT_NOK(s); + ASSERT_TRUE(s.IsInvalidArgument()); ASSERT_EQ(table_opt.cache_index_and_filter_blocks, new_opt.cache_index_and_filter_blocks); ASSERT_EQ(table_opt.filter_policy, new_opt.filter_policy); @@ -1021,18 +1043,22 @@ TEST_F(OptionsTest, GetPlainTableOptionsFromString) { ASSERT_TRUE(new_opt.store_index_in_file); // unknown option - ASSERT_NOK(GetPlainTableOptionsFromString( + Status s = GetPlainTableOptionsFromString( config_options, table_opt, "user_key_len=66;bloom_bits_per_key=20;hash_table_ratio=0.5;" "bad_option=1", - &new_opt)); + &new_opt); + ASSERT_NOK(s); + ASSERT_TRUE(s.IsInvalidArgument()); // unrecognized EncodingType - ASSERT_NOK(GetPlainTableOptionsFromString( + s = GetPlainTableOptionsFromString( config_options, table_opt, "user_key_len=66;bloom_bits_per_key=20;hash_table_ratio=0.5;" "encoding_type=kPrefixXX", - &new_opt)); + &new_opt); + ASSERT_NOK(s); + ASSERT_TRUE(s.IsInvalidArgument()); } #endif // !ROCKSDB_LITE @@ -1151,23 +1177,29 @@ TEST_F(OptionsTest, GetOptionsFromStringTest) { base_options.dump_malloc_stats = false; base_options.write_buffer_size = 1024; Options bad_options = new_options; - ASSERT_NOK(GetOptionsFromString(config_options, base_options, + Status s = GetOptionsFromString(config_options, base_options, "create_if_missing=XX;dump_malloc_stats=true", - &bad_options)); + &bad_options); + ASSERT_NOK(s); + ASSERT_TRUE(s.IsInvalidArgument()); ASSERT_EQ(bad_options.dump_malloc_stats, false); bad_options = new_options; - ASSERT_NOK(GetOptionsFromString(config_options, base_options, - "write_buffer_size=XX;dump_malloc_stats=true", - &bad_options)); + s = GetOptionsFromString(config_options, base_options, + "write_buffer_size=XX;dump_malloc_stats=true", + &bad_options); + ASSERT_NOK(s); + ASSERT_TRUE(s.IsInvalidArgument()); + ASSERT_EQ(bad_options.dump_malloc_stats, false); // Test a bad value for a TableFactory Option returns a failure bad_options = new_options; - ASSERT_NOK(GetOptionsFromString(config_options, base_options, - "write_buffer_size=16;dump_malloc_stats=true" - "block_based_table_factory={block_size=XX;};", - &bad_options)); + s = GetOptionsFromString(config_options, base_options, + "write_buffer_size=16;dump_malloc_stats=true" + "block_based_table_factory={block_size=XX;};", + &bad_options); + ASSERT_TRUE(s.IsInvalidArgument()); ASSERT_EQ(bad_options.dump_malloc_stats, false); ASSERT_EQ(bad_options.write_buffer_size, 1024); diff --git a/table/block_based/block_based_table_factory.cc b/table/block_based/block_based_table_factory.cc index 1881b8ac8..9e789a6ec 100644 --- a/table/block_based/block_based_table_factory.cc +++ b/table/block_based/block_based_table_factory.cc @@ -762,8 +762,14 @@ Status GetBlockBasedTableOptionsFromString( if (!s.ok()) { return s; } - return GetBlockBasedTableOptionsFromMap(config_options, table_options, - opts_map, new_table_options); + s = GetBlockBasedTableOptionsFromMap(config_options, table_options, opts_map, + new_table_options); + // Translate any errors (NotFound, NotSupported, to InvalidArgument + if (s.ok() || s.IsInvalidArgument()) { + return s; + } else { + return Status::InvalidArgument(s.getState()); + } } Status GetBlockBasedTableOptionsFromMap( diff --git a/table/plain/plain_table_factory.cc b/table/plain/plain_table_factory.cc index f465de66f..e0d0e69f6 100644 --- a/table/plain/plain_table_factory.cc +++ b/table/plain/plain_table_factory.cc @@ -142,8 +142,14 @@ Status GetPlainTableOptionsFromString(const ConfigOptions& config_options, return s; } - return GetPlainTableOptionsFromMap(config_options, table_options, opts_map, - new_table_options); + s = GetPlainTableOptionsFromMap(config_options, table_options, opts_map, + new_table_options); + // Translate any errors (NotFound, NotSupported, to InvalidArgument + if (s.ok() || s.IsInvalidArgument()) { + return s; + } else { + return Status::InvalidArgument(s.getState()); + } } Status GetMemTableRepFactoryFromString( diff --git a/utilities/options/options_util.cc b/utilities/options/options_util.cc index 3aa8625be..353725552 100644 --- a/utilities/options/options_util.cc +++ b/utilities/options/options_util.cc @@ -66,8 +66,9 @@ Status GetLatestOptionsFileName(const std::string& dbpath, std::vector file_names; s = env->GetChildren(dbpath, &file_names); if (s.IsNotFound()) { - return Status::PathNotFound("No options files found in the DB directory.", - dbpath); + return Status::NotFound(Status::kPathNotFound, + "No options files found in the DB directory.", + dbpath); } else if (!s.ok()) { return s; } @@ -82,8 +83,9 @@ Status GetLatestOptionsFileName(const std::string& dbpath, } } if (latest_file_name.size() == 0) { - return Status::PathNotFound("No options files found in the DB directory.", - dbpath); + return Status::NotFound(Status::kPathNotFound, + "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 ccfef3471..06ae9e62c 100644 --- a/utilities/options/options_util_test.cc +++ b/utilities/options/options_util_test.cc @@ -384,24 +384,29 @@ TEST_F(OptionsUtilTest, LatestOptionsNotFound) { ASSERT_NOK(options.env->GetChildren(dbname_, &children)); s = GetLatestOptionsFileName(dbname_, options.env, &options_file_name); + ASSERT_TRUE(s.IsNotFound()); ASSERT_TRUE(s.IsPathNotFound()); s = LoadLatestOptions(dbname_, options.env, &options, &cf_descs); + ASSERT_TRUE(s.IsNotFound()); 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.IsNotFound()); 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.IsNotFound()); ASSERT_TRUE(s.IsPathNotFound()); s = LoadLatestOptions(dbname_, options.env, &options, &cf_descs); + ASSERT_TRUE(s.IsNotFound()); ASSERT_TRUE(s.IsPathNotFound()); // Finally, test where a file exists but is not an "Options" file @@ -410,9 +415,11 @@ TEST_F(OptionsUtilTest, LatestOptionsNotFound) { options.env->NewWritableFile(dbname_ + "/temp.txt", &file, EnvOptions())); ASSERT_OK(file->Close()); s = GetLatestOptionsFileName(dbname_, options.env, &options_file_name); + ASSERT_TRUE(s.IsNotFound()); ASSERT_TRUE(s.IsPathNotFound()); s = LoadLatestOptions(config_opts, dbname_, &options, &cf_descs); + ASSERT_TRUE(s.IsNotFound()); ASSERT_TRUE(s.IsPathNotFound()); ASSERT_OK(options.env->DeleteFile(dbname_ + "/temp.txt")); ASSERT_OK(options.env->DeleteDir(dbname_)); @@ -544,7 +551,7 @@ TEST_F(OptionsUtilTest, BadLatestOptions) { ROCKSDB_MINOR, "unknown_db_opt=true", ""); s = LoadLatestOptions(config_opts, dbname_, &db_opts, &cf_descs); ASSERT_NOK(s); - ASSERT_TRUE(s.IsNotFound()); + ASSERT_TRUE(s.IsInvalidArgument()); ASSERT_OK(LoadLatestOptions(ignore_opts, dbname_, &db_opts, &cf_descs)); // Write an options file for a previous minor release with an unknown CF @@ -553,7 +560,7 @@ TEST_F(OptionsUtilTest, BadLatestOptions) { ROCKSDB_MINOR - 1, "", "unknown_cf_opt=true"); s = LoadLatestOptions(config_opts, dbname_, &db_opts, &cf_descs); ASSERT_NOK(s); - ASSERT_TRUE(s.IsNotFound()); + ASSERT_TRUE(s.IsInvalidArgument()); ASSERT_OK(LoadLatestOptions(ignore_opts, dbname_, &db_opts, &cf_descs)); // Write an options file for the current release with an unknown DB Option @@ -561,7 +568,7 @@ TEST_F(OptionsUtilTest, BadLatestOptions) { ROCKSDB_MINOR, "unknown_db_opt=true", ""); s = LoadLatestOptions(config_opts, dbname_, &db_opts, &cf_descs); ASSERT_NOK(s); - ASSERT_TRUE(s.IsNotFound()); + ASSERT_TRUE(s.IsInvalidArgument()); ASSERT_OK(LoadLatestOptions(ignore_opts, dbname_, &db_opts, &cf_descs)); // Write an options file for the current release with an unknown CF Option @@ -569,7 +576,7 @@ TEST_F(OptionsUtilTest, BadLatestOptions) { ROCKSDB_MINOR, "", "unknown_cf_opt=true"); s = LoadLatestOptions(config_opts, dbname_, &db_opts, &cf_descs); ASSERT_NOK(s); - ASSERT_TRUE(s.IsNotFound()); + ASSERT_TRUE(s.IsInvalidArgument()); ASSERT_OK(LoadLatestOptions(ignore_opts, dbname_, &db_opts, &cf_descs)); // Write an options file for the current release with an invalid DB Option