andreacavalli/rocksdb
0
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Revert Statuses returned from pre-Configurable options functions (#7563)

Browse Source 
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
This commit is contained in:
mrambacher 2020-10-20 11:51:51 -07:00 committed by akankshamahajan
parent d043387bdd
commit 59ddf78081
7 changed files with 134 additions and 57 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
include/rocksdb/status.h
View File

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

31
options/options_helper.cc
View File

@ -724,9 +724,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(
@ -773,9 +779,15 @@ Status GetDBOptionsFromMap(
assert(new_options);
*new_options = base_options;
auto config = DBOptionsAsConfigurable(base_options);
return ConfigureFromMap<DBOptions>(config_options, opts_map,
OptionsHelper::kDBOptionsName,
config.get(), new_options);
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,
@ -841,7 +853,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<std::string, EncodingType>

106
options/options_test.cc
View File

@ -302,8 +302,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(
@ -311,8 +314,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));
@ -397,22 +401,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));
@ -783,7 +794,10 @@ TEST_F(OptionsTest, OldInterfaceTest) {
ASSERT_EQ(new_db_opt.paranoid_checks, 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,
@ -795,11 +809,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));
}
@ -844,19 +860,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);
@ -870,21 +890,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);
@ -1017,18 +1039,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
@ -1147,23 +1173,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);

10
table/block_based/block_based_table_factory.cc
View File

@ -731,8 +731,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(

10
table/plain/plain_table_factory.cc
View File

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

10
utilities/options/options_util.cc
View File

@ -66,8 +66,9 @@ 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.",
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();

15
utilities/options/options_util_test.cc
View File

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