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
main
mrambacher 4 years ago committed by Facebook GitHub Bot
parent fc9b416013
commit 1eda625eab
  1. 9
      include/rocksdb/status.h
  2. 25
      options/options_helper.cc
  3. 100
      options/options_test.cc
  4. 10
      table/block_based/block_based_table_factory.cc
  5. 8
      table/plain/plain_table_factory.cc
  6. 6
      utilities/options/options_util.cc
  7. 15
      utilities/options/options_util_test.cc

@ -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

@ -727,9 +727,15 @@ Status GetColumnFamilyOptionsFromMap(
*new_options = base_options;
const auto config = CFOptionsAsConfigurable(base_options);
return ConfigureFromMap<ColumnFamilyOptions>(config_options, opts_map,
OptionsHelper::kCFOptionsName,
config.get(), new_options);
Status s = ConfigureFromMap<ColumnFamilyOptions>(
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<DBOptions>(config_options, opts_map,
Status s = ConfigureFromMap<DBOptions>(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);
}
}
// Translate any errors (NotFound, NotSupported, to InvalidArgument
if (s.ok() || s.IsInvalidArgument()) {
return s;
} else {
return Status::InvalidArgument(s.getState());
}
}
std::unordered_map<std::string, EncodingType>

@ -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<bool>(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,
s = GetOptionsFromString(config_options, base_options,
"write_buffer_size=XX;dump_malloc_stats=true",
&bad_options));
&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,
s = GetOptionsFromString(config_options, base_options,
"write_buffer_size=16;dump_malloc_stats=true"
"block_based_table_factory={block_size=XX;};",
&bad_options));
&bad_options);
ASSERT_TRUE(s.IsInvalidArgument());
ASSERT_EQ(bad_options.dump_malloc_stats, false);
ASSERT_EQ(bad_options.write_buffer_size, 1024);

@ -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(

@ -142,8 +142,14 @@ Status GetPlainTableOptionsFromString(const ConfigOptions& config_options,
return s;
}
return GetPlainTableOptionsFromMap(config_options, table_options, opts_map,
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(

@ -66,7 +66,8 @@ Status GetLatestOptionsFileName(const std::string& dbpath,
std::vector<std::string> file_names;
s = env->GetChildren(dbpath, &file_names);
if (s.IsNotFound()) {
return Status::PathNotFound("No options files found in the DB directory.",
return Status::NotFound(Status::kPathNotFound,
"No options files found in the DB directory.",
dbpath);
} else if (!s.ok()) {
return s;
@ -82,7 +83,8 @@ Status GetLatestOptionsFileName(const std::string& dbpath,
}
}
if (latest_file_name.size() == 0) {
return Status::PathNotFound("No options files found in the DB directory.",
return Status::NotFound(Status::kPathNotFound,
"No options files found in the DB directory.",
dbpath);
}
*options_file_name = latest_file_name;

@ -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

Loading…
Cancel
Save