empiredan commented on code in PR #1875:
URL:
https://github.com/apache/incubator-pegasus/pull/1875#discussion_r1479498419
##########
src/nfs/nfs_client_impl.cpp:
##########
@@ -69,14 +69,11 @@ DSN_DEFINE_uint32(nfs,
nfs_copy_block_bytes,
4 * 1024 * 1024,
"max block size (bytes) for each network copy");
-DSN_DEFINE_uint32(
- nfs,
- max_copy_rate_megabytes_per_disk,
- 0,
- "max rate per disk of copying from remote node(MB/s), zero means disable
rate limiter");
+static const char *max_copy_rate_megabytes_per_disk_desc =
Review Comment:
```suggestion
static const char *kMaxCopyRateMegaBytesPerDiskDesc =
```
##########
src/utils/command_manager.cpp:
##########
@@ -43,31 +43,34 @@ command_manager::register_command(const
std::vector<std::string> &commands,
const std::string &help_long,
command_handler handler)
{
- utils::auto_write_lock l(_lock);
- bool is_valid_cmd = false;
- for (const std::string &cmd : commands) {
- if (!cmd.empty()) {
- is_valid_cmd = true;
- CHECK(_handlers.find(cmd) == _handlers.end(), "command '{}'
already regisered", cmd);
- }
- }
- CHECK(is_valid_cmd, "should not register empty command");
-
- command_instance *c = new command_instance();
+ auto *c = new command_instance();
c->commands = commands;
c->help_short = help_one_line;
c->help_long = help_long;
- c->handler = handler;
+ c->handler = std::move(handler);
- for (const std::string &cmd : commands) {
- if (!cmd.empty()) {
- _handlers[cmd] = c;
- }
+ utils::auto_write_lock l(_lock);
+ for (const auto &cmd : commands) {
+ CHECK(!cmd.empty(), "should not register empty command");
+ CHECK(_handlers.insert(std::make_pair(cmd, c)).second,
Review Comment:
```suggestion
CHECK(_handlers. emplace(cmd, c).second,
```
##########
src/nfs/nfs_client_impl.cpp:
##########
@@ -582,32 +579,16 @@ void nfs_client_impl::register_cli_commands()
{
static std::once_flag flag;
std::call_once(flag, [&]() {
- _nfs_max_copy_rate_megabytes_cmd =
dsn::command_manager::instance().register_command(
- {"nfs.max_copy_rate_megabytes_per_disk"},
- "nfs.max_copy_rate_megabytes_per_disk [num]",
- "control the max rate(MB/s) for one disk to copy file from remote
node",
- [](const std::vector<std::string> &args) {
- std::string result("OK");
-
- if (args.empty()) {
- return
std::to_string(FLAGS_max_copy_rate_megabytes_per_disk);
- }
-
- int32_t max_copy_rate_megabytes = 0;
- if (!dsn::buf2int32(args[0], max_copy_rate_megabytes) ||
- max_copy_rate_megabytes <= 0) {
- return std::string("ERR: invalid arguments");
- }
-
- uint32_t max_copy_rate_bytes = max_copy_rate_megabytes << 20;
- if (max_copy_rate_bytes <= FLAGS_nfs_copy_block_bytes) {
- result = std::string("ERR:
max_copy_rate_bytes(max_copy_rate_megabytes << 20) "
- "should be greater than
nfs_copy_block_bytes:")
-
.append(std::to_string(FLAGS_nfs_copy_block_bytes));
- return result;
- }
- FLAGS_max_copy_rate_megabytes_per_disk =
max_copy_rate_megabytes;
- return result;
+ _nfs_max_copy_rate_megabytes_cmd =
dsn::command_manager::instance().register_int_command(
+ FLAGS_max_copy_rate_megabytes_per_disk,
+ FLAGS_max_copy_rate_megabytes_per_disk,
+ "nfs.max_copy_rate_megabytes_per_disk",
+ fmt::format("{}, "
+ "should be less than 'nfs_copy_block_bytes' which is
{}",
+ max_copy_rate_megabytes_per_disk_desc,
Review Comment:
```suggestion
kMaxCopyRateMegaBytesPerDiskDesc,
```
##########
src/replica/replica_stub.cpp:
##########
@@ -2239,33 +2204,11 @@ void replica_stub::register_ctrl_command()
#elif defined(DSN_USE_JEMALLOC)
register_jemalloc_ctrl_command();
#endif
- // TODO(yingchun): use http
- _cmds.emplace_back(::dsn::command_manager::instance().register_command(
- {"replica.max-concurrent-bulk-load-downloading-count"},
- "replica.max-concurrent-bulk-load-downloading-count [num |
DEFAULT]",
- "control stub max_concurrent_bulk_load_downloading_count",
- [this](const std::vector<std::string> &args) {
- std::string result("OK");
- if (args.empty()) {
- result = "max_concurrent_bulk_load_downloading_count=" +
-
std::to_string(_max_concurrent_bulk_load_downloading_count);
- return result;
- }
-
- if (args[0] == "DEFAULT") {
- _max_concurrent_bulk_load_downloading_count =
- _options.max_concurrent_bulk_load_downloading_count;
- return result;
- }
-
- int32_t count = 0;
- if (!dsn::buf2int32(args[0], count) || count <= 0) {
- result = std::string("ERR: invalid arguments");
- } else {
- _max_concurrent_bulk_load_downloading_count = count;
- }
- return result;
- }));
+
_cmds.emplace_back(::dsn::command_manager::instance().register_int_command(
Review Comment:
Should we add validator to ensure that
`max_concurrent_bulk_load_downloading_count` > 0 ?
##########
src/replica/replica_stub.cpp:
##########
@@ -86,6 +85,13 @@
#include "remote_cmd/remote_command.h"
#include "utils/fail_point.h"
+static const char *max_concurrent_bulk_load_downloading_count_desc =
+ "The maximum concurrent bulk load downloading replica count";
+DSN_DEFINE_int32(replication,
+ max_concurrent_bulk_load_downloading_count,
+ 5,
+ max_concurrent_bulk_load_downloading_count_desc);
Review Comment:
```suggestion
kMaxConcurrentBulkLoadDownloadingCountDesc);
```
##########
src/replica/replica_stub.cpp:
##########
@@ -86,6 +85,13 @@
#include "remote_cmd/remote_command.h"
#include "utils/fail_point.h"
+static const char *max_concurrent_bulk_load_downloading_count_desc =
Review Comment:
```suggestion
static const char *kMaxConcurrentBulkLoadDownloadingCountDesc =
```
##########
src/nfs/nfs_server_impl.cpp:
##########
@@ -261,26 +259,11 @@ void nfs_service_impl::register_cli_commands()
{
static std::once_flag flag;
std::call_once(flag, [&]() {
- _nfs_max_send_rate_megabytes_cmd =
dsn::command_manager::instance().register_command(
- {"nfs.max_send_rate_megabytes_per_disk"},
- "nfs.max_send_rate_megabytes_per_disk [num]",
- "control the max rate(MB/s) for one disk to send file to remote
node",
- [](const std::vector<std::string> &args) {
- std::string result("OK");
-
- if (args.empty()) {
- return
std::to_string(FLAGS_max_send_rate_megabytes_per_disk);
- }
-
- int32_t max_send_rate_megabytes = 0;
- if (!dsn::buf2int32(args[0], max_send_rate_megabytes) ||
- max_send_rate_megabytes <= 0) {
- return std::string("ERR: invalid arguments");
- }
-
- FLAGS_max_send_rate_megabytes_per_disk =
max_send_rate_megabytes;
- return result;
- });
+ _nfs_max_send_rate_megabytes_cmd =
dsn::command_manager::instance().register_int_command(
+ FLAGS_max_send_rate_megabytes_per_disk,
+ FLAGS_max_send_rate_megabytes_per_disk,
+ "nfs.max_send_rate_megabytes_per_disk",
+ max_send_rate_megabytes_per_disk_desc);
Review Comment:
```suggestion
kMaxSendRateMegaBytesPerDiskDesc);
```
##########
src/nfs/nfs_server_impl.cpp:
##########
@@ -62,11 +61,10 @@ class disk_file;
namespace service {
-DSN_DEFINE_uint32(
- nfs,
- max_send_rate_megabytes_per_disk,
- 0,
- "max rate per disk of send to remote node(MB/s)，zero means disable rate
limiter");
+static const char *max_send_rate_megabytes_per_disk_desc =
Review Comment:
```suggestion
static const char *kMaxSendRateMegaBytesPerDiskDesc =
```
##########
src/nfs/nfs_server_impl.cpp:
##########
@@ -62,11 +61,10 @@ class disk_file;
namespace service {
-DSN_DEFINE_uint32(
- nfs,
- max_send_rate_megabytes_per_disk,
- 0,
- "max rate per disk of send to remote node(MB/s)，zero means disable rate
limiter");
+static const char *max_send_rate_megabytes_per_disk_desc =
+ "The maximum bandwidth (MB/s) of reading data per local disk "
+ "when transferring data to remote node, 0 means no limit";
+DSN_DEFINE_int64(nfs, max_send_rate_megabytes_per_disk, 0,
max_send_rate_megabytes_per_disk_desc);
Review Comment:
```suggestion
DSN_DEFINE_int64(nfs, max_send_rate_megabytes_per_disk, 0,
kMaxSendRateMegaBytesPerDiskDesc);
```
##########
src/nfs/nfs_server_impl.cpp:
##########
@@ -261,26 +259,11 @@ void nfs_service_impl::register_cli_commands()
{
static std::once_flag flag;
std::call_once(flag, [&]() {
- _nfs_max_send_rate_megabytes_cmd =
dsn::command_manager::instance().register_command(
- {"nfs.max_send_rate_megabytes_per_disk"},
- "nfs.max_send_rate_megabytes_per_disk [num]",
- "control the max rate(MB/s) for one disk to send file to remote
node",
- [](const std::vector<std::string> &args) {
- std::string result("OK");
-
- if (args.empty()) {
- return
std::to_string(FLAGS_max_send_rate_megabytes_per_disk);
- }
-
- int32_t max_send_rate_megabytes = 0;
- if (!dsn::buf2int32(args[0], max_send_rate_megabytes) ||
- max_send_rate_megabytes <= 0) {
- return std::string("ERR: invalid arguments");
- }
-
- FLAGS_max_send_rate_megabytes_per_disk =
max_send_rate_megabytes;
- return result;
- });
+ _nfs_max_send_rate_megabytes_cmd =
dsn::command_manager::instance().register_int_command(
Review Comment:
Should we add a validator for both flag and command to ensure that
`max_send_rate_megabytes_per_disk_desc` > 0, since I saw assertions in
`src/utils/TokenBucket.h`:
```c++
boost::optional<double> consumeWithBorrowNonBlocking(double toConsume,
double rate,
double burstSize,
double nowInSeconds
= defaultClockNow())
{
assert(rate > 0);
assert(burstSize > 0);
......
}
```
##########
src/nfs/nfs_client_impl.cpp:
##########
@@ -69,14 +69,11 @@ DSN_DEFINE_uint32(nfs,
nfs_copy_block_bytes,
4 * 1024 * 1024,
"max block size (bytes) for each network copy");
-DSN_DEFINE_uint32(
- nfs,
- max_copy_rate_megabytes_per_disk,
- 0,
- "max rate per disk of copying from remote node(MB/s), zero means disable
rate limiter");
+static const char *max_copy_rate_megabytes_per_disk_desc =
+ "The maximum bandwidth (MB/s) of writing data per local disk when copying
from remote node, 0 "
+ "means no limit";
+DSN_DEFINE_int64(nfs, max_copy_rate_megabytes_per_disk, 0,
max_copy_rate_megabytes_per_disk_desc);
Review Comment:
```suggestion
DSN_DEFINE_int64(nfs, max_copy_rate_megabytes_per_disk, 0,
kMaxCopyRateMegaBytesPerDiskDesc);
```
##########
src/nfs/nfs_client_impl.cpp:
##########
@@ -582,32 +579,16 @@ void nfs_client_impl::register_cli_commands()
{
static std::once_flag flag;
std::call_once(flag, [&]() {
- _nfs_max_copy_rate_megabytes_cmd =
dsn::command_manager::instance().register_command(
- {"nfs.max_copy_rate_megabytes_per_disk"},
- "nfs.max_copy_rate_megabytes_per_disk [num]",
- "control the max rate(MB/s) for one disk to copy file from remote
node",
- [](const std::vector<std::string> &args) {
- std::string result("OK");
-
- if (args.empty()) {
- return
std::to_string(FLAGS_max_copy_rate_megabytes_per_disk);
- }
-
- int32_t max_copy_rate_megabytes = 0;
- if (!dsn::buf2int32(args[0], max_copy_rate_megabytes) ||
- max_copy_rate_megabytes <= 0) {
- return std::string("ERR: invalid arguments");
- }
-
- uint32_t max_copy_rate_bytes = max_copy_rate_megabytes << 20;
- if (max_copy_rate_bytes <= FLAGS_nfs_copy_block_bytes) {
- result = std::string("ERR:
max_copy_rate_bytes(max_copy_rate_megabytes << 20) "
- "should be greater than
nfs_copy_block_bytes:")
-
.append(std::to_string(FLAGS_nfs_copy_block_bytes));
- return result;
- }
- FLAGS_max_copy_rate_megabytes_per_disk =
max_copy_rate_megabytes;
- return result;
+ _nfs_max_copy_rate_megabytes_cmd =
dsn::command_manager::instance().register_int_command(
+ FLAGS_max_copy_rate_megabytes_per_disk,
+ FLAGS_max_copy_rate_megabytes_per_disk,
+ "nfs.max_copy_rate_megabytes_per_disk",
+ fmt::format("{}, "
+ "should be less than 'nfs_copy_block_bytes' which is
{}",
+ max_copy_rate_megabytes_per_disk_desc,
+ FLAGS_nfs_copy_block_bytes),
+ [](int64_t new_value) -> bool {
+ return new_value == 0 || (new_value << 20) >
FLAGS_nfs_copy_block_bytes;
Review Comment:
Could we define a function named something like
`check_max_copy_rate_megabytes_per_disk()` to be called by both the command
validator and group validator ?
##########
src/replica/replica_stub.cpp:
##########
@@ -2201,32 +2186,12 @@ void replica_stub::register_ctrl_command()
return std::string(buf);
}));
- _cmds.emplace_back(::dsn::command_manager::instance().register_command(
- {"replica.mem-release-max-reserved-percentage"},
- "replica.mem-release-max-reserved-percentage [num | DEFAULT]",
+
_cmds.emplace_back(::dsn::command_manager::instance().register_int_command(
+ _mem_release_max_reserved_mem_percentage,
+ FLAGS_mem_release_max_reserved_mem_percentage,
+ "replica.mem-release-max-reserved-percentage",
"control tcmalloc max reserved but not-used memory percentage",
- [this](const std::vector<std::string> &args) {
- std::string result("OK");
- if (args.empty()) {
- // show current value
- result = "mem-release-max-reserved-percentage = " +
-
std::to_string(_mem_release_max_reserved_mem_percentage);
- return result;
- }
- if (args[0] == "DEFAULT") {
- // set to default value
- _mem_release_max_reserved_mem_percentage =
- FLAGS_mem_release_max_reserved_mem_percentage;
- return result;
- }
- int32_t percentage = 0;
- if (!dsn::buf2int32(args[0], percentage) || percentage <= 0 ||
percentage > 100) {
- result = std::string("ERR: invalid arguments");
- } else {
- _mem_release_max_reserved_mem_percentage = percentage;
- }
- return result;
- }));
+ [](int32_t new_value) -> bool { return new_value > 0 && new_value
<= 100; }));
Review Comment:
Should this also be added for flag validator ?
##########
src/replica/replica_stub.cpp:
##########
@@ -2239,33 +2204,11 @@ void replica_stub::register_ctrl_command()
#elif defined(DSN_USE_JEMALLOC)
register_jemalloc_ctrl_command();
#endif
- // TODO(yingchun): use http
- _cmds.emplace_back(::dsn::command_manager::instance().register_command(
- {"replica.max-concurrent-bulk-load-downloading-count"},
- "replica.max-concurrent-bulk-load-downloading-count [num |
DEFAULT]",
- "control stub max_concurrent_bulk_load_downloading_count",
- [this](const std::vector<std::string> &args) {
- std::string result("OK");
- if (args.empty()) {
- result = "max_concurrent_bulk_load_downloading_count=" +
-
std::to_string(_max_concurrent_bulk_load_downloading_count);
- return result;
- }
-
- if (args[0] == "DEFAULT") {
- _max_concurrent_bulk_load_downloading_count =
- _options.max_concurrent_bulk_load_downloading_count;
- return result;
- }
-
- int32_t count = 0;
- if (!dsn::buf2int32(args[0], count) || count <= 0) {
- result = std::string("ERR: invalid arguments");
- } else {
- _max_concurrent_bulk_load_downloading_count = count;
- }
- return result;
- }));
+
_cmds.emplace_back(::dsn::command_manager::instance().register_int_command(
+ FLAGS_max_concurrent_bulk_load_downloading_count,
+ FLAGS_max_concurrent_bulk_load_downloading_count,
+ "replica.max-concurrent-bulk-load-downloading-count",
+ max_concurrent_bulk_load_downloading_count_desc));
Review Comment:
```suggestion
kMaxConcurrentBulkLoadDownloadingCountDesc));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
