Skip to content

Commit

Permalink
Thank you empiredan for the very detailed suggestion, I modified my c…
Browse files Browse the repository at this point in the history 
…ode.

Mainly the following aspects:
1. Improve the unit test to ensure that the new option can also be added correctly.
2. Add getter and setter methods for option.
  • Loading branch information
@ruojieranyishen
ruojieranyishen committed Jul 6, 2023
1 parent 79b77aa commit 4b838e2
Show file tree
Hide file tree
Showing 7 changed files with 289 additions and 75 deletions.
37 changes: 37 additions & 0 deletions src/base/pegasus_const.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,13 @@

#include "pegasus_const.h"

#include <fmt/core.h>
#include <rocksdb/options.h>
#include <stddef.h>
#include <stdint.h>

#include "utils/string_conv.h"

namespace pegasus {

// should be same with items in dsn::backup_restore_constant
Expand Down Expand Up @@ -112,4 +119,34 @@ const std::set<std::string> ROCKSDB_DYNAMIC_OPTIONS = {
const std::set<std::string> ROCKSDB_STATIC_OPTIONS = {
ROCKSDB_NUM_LEVELS,
};

const std::unordered_map<std::string, cf_opts_setter> cf_opts_setters = {
{ROCKSDB_WRITE_BUFFER_SIZE,
[](const std::string &str, rocksdb::ColumnFamilyOptions &option) -> bool {
uint64_t val = 0;
if (!dsn::buf2uint64(str, val))
return false;
option.write_buffer_size = static_cast<size_t>(val);
return true;
}},
{ROCKSDB_NUM_LEVELS,
[](const std::string &str, rocksdb::ColumnFamilyOptions &option) -> bool {
int32_t val = 0;
if (!dsn::buf2int32(str, val))
return false;
option.num_levels = val;
return true;
}},
};

const std::unordered_map<std::string, cf_opts_getter> cf_opts_getters = {
{ROCKSDB_WRITE_BUFFER_SIZE,
[](/*out*/ std::string &str, const rocksdb::ColumnFamilyOptions &option) {
str = fmt::format("{}", option.write_buffer_size);
}},
{ROCKSDB_NUM_LEVELS,
[](/*out*/ std::string &str, const rocksdb::ColumnFamilyOptions &option) {
str = fmt::format("{}", option.num_levels);
}},
};
} // namespace pegasus
13 changes: 13 additions & 0 deletions src/base/pegasus_const.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,14 @@

#pragma once

#include <functional>
#include <set>
#include <string>
#include <unordered_map>

namespace rocksdb {
struct ColumnFamilyOptions;
} // namespace rocksdb

namespace pegasus {

Expand Down Expand Up @@ -81,4 +87,11 @@ extern const std::string ROCKSDB_NUM_LEVELS;
extern const std::set<std::string> ROCKSDB_DYNAMIC_OPTIONS;

extern const std::set<std::string> ROCKSDB_STATIC_OPTIONS;

using cf_opts_setter = std::function<bool(const std::string &, rocksdb::ColumnFamilyOptions &)>;
extern const std::unordered_map<std::string, cf_opts_setter> cf_opts_setters;

using cf_opts_getter =
std::function<void(/*out*/ std::string &, const rocksdb::ColumnFamilyOptions &)>;
extern const std::unordered_map<std::string, cf_opts_getter> cf_opts_getters;
} // namespace pegasus
4 changes: 2 additions & 2 deletions src/meta/app_env_validator.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -180,9 +180,9 @@ bool check_rocksdb_write_buffer_size(const std::string &env_value, std::string &
hint_message = fmt::format("rocksdb.write_buffer_size cannot set this val: {}", env_value);
return false;
}
if (val < (32 << 20) || val > (512 << 20)) {
if (val < (16 << 20) || val > (512 << 20)) {
hint_message =
fmt::format("rocksdb.write_buffer_size suggest set val in range [33554432, 536870912]");
fmt::format("rocksdb.write_buffer_size suggest set val in range [16777216, 536870912]");
return false;
}
return true;
Expand Down
17 changes: 15 additions & 2 deletions src/meta/test/meta_app_envs_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include <gtest/gtest.h>
#include <map>
#include <memory>
#include <set>
#include <string>

#include "common/replica_envs.h"
Expand Down Expand Up @@ -145,12 +146,12 @@ TEST_F(meta_app_envs_test, update_app_envs_test)
{replica_envs::ROCKSDB_WRITE_BUFFER_SIZE,
"100",
ERR_INVALID_PARAMETERS,
"rocksdb.write_buffer_size suggest set val in range [33554432, 536870912]",
"rocksdb.write_buffer_size suggest set val in range [16777216, 536870912]",
"67108864"},
{replica_envs::ROCKSDB_WRITE_BUFFER_SIZE,
"636870912",
ERR_INVALID_PARAMETERS,
"rocksdb.write_buffer_size suggest set val in range [33554432, 536870912]",
"rocksdb.write_buffer_size suggest set val in range [16777216, 536870912]",
"536870912"},
{replica_envs::ROCKSDB_WRITE_BUFFER_SIZE, "67108864", ERR_OK, "", "67108864"},
{replica_envs::MANUAL_COMPACT_PERIODIC_BOTTOMMOST_LEVEL_COMPACTION,
Expand Down Expand Up @@ -203,6 +204,18 @@ TEST_F(meta_app_envs_test, update_app_envs_test)
ASSERT_EQ(app->envs.at(test.env_key), test.expect_value);
}
}

{
// Make sure all rocksdb options of ROCKSDB_DYNAMIC_OPTIONS are tested.
// Hint: Mainly verify the update_rocksdb_dynamic_options function.
std::map<std::string, std::string> all_test_envs;
for (auto test : tests) {
all_test_envs[test.env_key] = test.env_value;
}
for (const auto &option : replica_envs::ROCKSDB_DYNAMIC_OPTIONS) {
ASSERT_TRUE(all_test_envs.find(option) != all_test_envs.end());
}
}
}

} // namespace replication
Expand Down
169 changes: 129 additions & 40 deletions src/meta/test/meta_app_operation_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include <iostream>
#include <map>
#include <memory>
#include <set>
#include <string>
#include <utility>
#include <vector>
Expand Down Expand Up @@ -68,7 +69,8 @@ class meta_app_operation_test : public meta_test_base
error_code create_app_test(int32_t partition_count,
int32_t replica_count,
bool success_if_exist,
const std::string &app_name)
const std::string &app_name,
const std::map<std::string, std::string> &envs = {})
{
configuration_create_app_request create_request;
configuration_create_app_response create_response;
Expand All @@ -78,6 +80,7 @@ class meta_app_operation_test : public meta_test_base
create_request.options.replica_count = replica_count;
create_request.options.success_if_exist = success_if_exist;
create_request.options.is_stateful = true;
create_request.options.envs = envs;

auto result = fake_create_app(_ss.get(), create_request);
fake_wait_rpc(result, create_response);
Expand Down Expand Up @@ -362,6 +365,12 @@ TEST_F(meta_app_operation_test, create_app)
// - wrong app_status dropping
// - create succeed with app_status dropped
// - create succeed with success_if_exist=true
// - wrong rocksdb.num_levels (< 1)
// - wrong rocksdb.num_levels (> 10)
// - create app with rocksdb.num_levels (= 5) succeed
// - wrong rocksdb.write_buffer_size (< (16<<20))
// - wrong rocksdb.write_buffer_size (> (512<<20))
// - create app with rocksdb.write_buffer_size (= (32<<20)) succeed
struct create_test
{
std::string app_name;
Expand All @@ -373,43 +382,106 @@ TEST_F(meta_app_operation_test, create_app)
bool success_if_exist;
app_status::type before_status;
error_code expected_err;
} tests[] = {{APP_NAME, -1, 3, 2, 3, 1, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS},
{APP_NAME, 0, 3, 2, 3, 1, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS},
{APP_NAME, 4, -1, 1, 3, 1, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS},
{APP_NAME, 4, 0, 1, 3, 1, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS},
{APP_NAME, 4, 6, 2, 4, 1, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS},
{APP_NAME, 4, 7, 2, 6, 1, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS},
{APP_NAME, 4, 6, 2, 5, 1, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS},
{APP_NAME, 4, 5, 2, 4, 1, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS},
{APP_NAME, 4, 4, 2, 3, 1, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS},
{APP_NAME, 4, 6, 2, 6, 1, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS},
{APP_NAME, 4, 6, 2, 7, 1, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS},
{APP_NAME + "_1", 4, 5, 2, 5, 1, false, app_status::AS_INVALID, ERR_OK},
{APP_NAME + "_2", 4, 5, 2, 6, 1, false, app_status::AS_INVALID, ERR_OK},
{APP_NAME + "_3", 4, 4, 2, 4, 1, false, app_status::AS_INVALID, ERR_OK},
{APP_NAME + "_4", 4, 4, 2, 6, 1, false, app_status::AS_INVALID, ERR_OK},
{APP_NAME + "_5", 4, 3, 2, 4, 1, false, app_status::AS_INVALID, ERR_OK},
{APP_NAME + "_6", 4, 4, 2, 5, 1, false, app_status::AS_INVALID, ERR_OK},
{APP_NAME, 4, 3, 2, 5, 4, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS},
{APP_NAME, 4, 3, 2, 4, 5, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS},
{APP_NAME, 4, 3, 2, 4, 4, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS},
{APP_NAME, 4, 3, 2, 2, 4, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS},
{APP_NAME, 4, 3, 2, 3, 4, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS},
{APP_NAME, 4, 4, 2, 3, 4, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS},
{APP_NAME + "_7", 4, 3, 2, 4, 3, false, app_status::AS_INVALID, ERR_OK},
{APP_NAME, 4, 1, 1, 0, 1, false, app_status::AS_INVALID, ERR_STATE_FREEZED},
{APP_NAME, 4, 2, 2, 1, 1, false, app_status::AS_INVALID, ERR_STATE_FREEZED},
{APP_NAME, 4, 3, 3, 2, 1, false, app_status::AS_INVALID, ERR_STATE_FREEZED},
{APP_NAME + "_8", 4, 3, 3, 3, 1, false, app_status::AS_INVALID, ERR_OK},
{APP_NAME + "_9", 4, 1, 1, 1, 1, false, app_status::AS_INVALID, ERR_OK},
{APP_NAME + "_10", 4, 2, 1, 2, 2, false, app_status::AS_INVALID, ERR_OK},
{APP_NAME, 4, 3, 2, 3, 3, false, app_status::AS_INVALID, ERR_OK},
{APP_NAME, 4, 3, 2, 3, 3, false, app_status::AS_INVALID, ERR_APP_EXIST},
{APP_NAME, 4, 3, 2, 3, 3, false, app_status::AS_CREATING, ERR_BUSY_CREATING},
{APP_NAME, 4, 3, 2, 3, 3, false, app_status::AS_RECALLING, ERR_BUSY_CREATING},
{APP_NAME, 4, 3, 2, 3, 3, false, app_status::AS_DROPPING, ERR_BUSY_DROPPING},
{APP_NAME, 4, 3, 2, 3, 3, false, app_status::AS_DROPPED, ERR_OK},
{APP_NAME, 4, 3, 2, 3, 3, true, app_status::AS_INVALID, ERR_OK}};
std::map<std::string, std::string> envs = {};
} tests[] = {
{APP_NAME, -1, 3, 2, 3, 1, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS},
{APP_NAME, 0, 3, 2, 3, 1, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS},
{APP_NAME, 4, -1, 1, 3, 1, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS},
{APP_NAME, 4, 0, 1, 3, 1, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS},
{APP_NAME, 4, 6, 2, 4, 1, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS},
{APP_NAME, 4, 7, 2, 6, 1, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS},
{APP_NAME, 4, 6, 2, 5, 1, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS},
{APP_NAME, 4, 5, 2, 4, 1, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS},
{APP_NAME, 4, 4, 2, 3, 1, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS},
{APP_NAME, 4, 6, 2, 6, 1, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS},
{APP_NAME, 4, 6, 2, 7, 1, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS},
{APP_NAME + "_1", 4, 5, 2, 5, 1, false, app_status::AS_INVALID, ERR_OK},
{APP_NAME + "_2", 4, 5, 2, 6, 1, false, app_status::AS_INVALID, ERR_OK},
{APP_NAME + "_3", 4, 4, 2, 4, 1, false, app_status::AS_INVALID, ERR_OK},
{APP_NAME + "_4", 4, 4, 2, 6, 1, false, app_status::AS_INVALID, ERR_OK},
{APP_NAME + "_5", 4, 3, 2, 4, 1, false, app_status::AS_INVALID, ERR_OK},
{APP_NAME + "_6", 4, 4, 2, 5, 1, false, app_status::AS_INVALID, ERR_OK},
{APP_NAME, 4, 3, 2, 5, 4, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS},
{APP_NAME, 4, 3, 2, 4, 5, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS},
{APP_NAME, 4, 3, 2, 4, 4, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS},
{APP_NAME, 4, 3, 2, 2, 4, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS},
{APP_NAME, 4, 3, 2, 3, 4, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS},
{APP_NAME, 4, 4, 2, 3, 4, false, app_status::AS_INVALID, ERR_INVALID_PARAMETERS},
{APP_NAME + "_7", 4, 3, 2, 4, 3, false, app_status::AS_INVALID, ERR_OK},
{APP_NAME, 4, 1, 1, 0, 1, false, app_status::AS_INVALID, ERR_STATE_FREEZED},
{APP_NAME, 4, 2, 2, 1, 1, false, app_status::AS_INVALID, ERR_STATE_FREEZED},
{APP_NAME, 4, 3, 3, 2, 1, false, app_status::AS_INVALID, ERR_STATE_FREEZED},
{APP_NAME + "_8", 4, 3, 3, 3, 1, false, app_status::AS_INVALID, ERR_OK},
{APP_NAME + "_9", 4, 1, 1, 1, 1, false, app_status::AS_INVALID, ERR_OK},
{APP_NAME + "_10", 4, 2, 1, 2, 2, false, app_status::AS_INVALID, ERR_OK},
{APP_NAME, 4, 3, 2, 3, 3, false, app_status::AS_INVALID, ERR_OK},
{APP_NAME, 4, 3, 2, 3, 3, false, app_status::AS_INVALID, ERR_APP_EXIST},
{APP_NAME, 4, 3, 2, 3, 3, false, app_status::AS_CREATING, ERR_BUSY_CREATING},
{APP_NAME, 4, 3, 2, 3, 3, false, app_status::AS_RECALLING, ERR_BUSY_CREATING},
{APP_NAME, 4, 3, 2, 3, 3, false, app_status::AS_DROPPING, ERR_BUSY_DROPPING},
{APP_NAME, 4, 3, 2, 3, 3, false, app_status::AS_DROPPED, ERR_OK},
{APP_NAME, 4, 3, 2, 3, 3, true, app_status::AS_INVALID, ERR_OK},
{APP_NAME,
4,
3,
2,
3,
3,
false,
app_status::AS_INVALID,
ERR_INVALID_PARAMETERS,
{{"rocksdb.num_levels", "0"}}},
{APP_NAME,
4,
3,
2,
3,
3,
false,
app_status::AS_INVALID,
ERR_INVALID_PARAMETERS,
{{"rocksdb.num_levels", "11"}}},
{APP_NAME + "_11",
4,
3,
2,
3,
3,
false,
app_status::AS_INVALID,
ERR_OK,
{{"rocksdb.num_levels", "5"}}},
{APP_NAME,
4,
3,
2,
3,
3,
false,
app_status::AS_INVALID,
ERR_INVALID_PARAMETERS,
{{"rocksdb.write_buffer_size", "1000"}}},
{APP_NAME,
4,
3,
2,
3,
3,
false,
app_status::AS_INVALID,
ERR_INVALID_PARAMETERS,
{{"rocksdb.write_buffer_size", "1073741824"}}},
{APP_NAME + "_12",
4,
3,
2,
3,
3,
false,
app_status::AS_INVALID,
ERR_OK,
{{"rocksdb.write_buffer_size", "33554432"}}},
};

clear_nodes();

Expand Down Expand Up @@ -453,13 +525,30 @@ TEST_F(meta_app_operation_test, create_app)
} else if (test.before_status != app_status::AS_INVALID) {
update_app_status(test.before_status);
}
auto err = create_app_test(
test.partition_count, test.replica_count, test.success_if_exist, test.app_name);
auto err = create_app_test(test.partition_count,
test.replica_count,
test.success_if_exist,
test.app_name,
test.envs);
ASSERT_EQ(err, test.expected_err);

_ms->set_node_state(nodes, true);
}

{
// Make sure all rocksdb options of ROCKSDB_DYNAMIC_OPTIONS and ROCKSDB_STATIC_OPTIONS are
// tested. Hint: Mainly verify the validate_app_envs function.
std::map<std::string, std::string> all_test_envs;
for (auto test : tests) {
all_test_envs.insert(test.envs.begin(), test.envs.end());
}
for (const auto &option : replica_envs::ROCKSDB_DYNAMIC_OPTIONS) {
ASSERT_TRUE(all_test_envs.find(option) != all_test_envs.end());
}
for (const auto &option : replica_envs::ROCKSDB_STATIC_OPTIONS) {
ASSERT_TRUE(all_test_envs.find(option) != all_test_envs.end());
}
}
// set FLAGS_min_allowed_replica_count successfully
res = update_flag("min_allowed_replica_count", "2");
ASSERT_TRUE(res.is_ok());
Expand Down
Loading

0 comments on commit 4b838e2

Please sign in to comment.