You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pegasus.apache.org by GitBox <gi...@apache.org> on 2022/10/26 03:37:29 UTC
[GitHub] [incubator-pegasus] acelyc111 opened a new pull request, #1205: refactor(macro): Use CHECK to replace dassert_f/dassert
acelyc111 opened a new pull request, #1205:
URL: https://github.com/apache/incubator-pegasus/pull/1205
https://github.com/apache/incubator-pegasus/issues/1204
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] empiredan commented on a diff in pull request #1205: refactor(macro): Use CHECK to replace dassert_f/dassert
Posted by GitBox <gi...@apache.org>.
empiredan commented on code in PR #1205:
URL: https://github.com/apache/incubator-pegasus/pull/1205#discussion_r1006356509
##########
src/base/value_schema_manager.cpp:
##########
@@ -57,7 +57,7 @@ value_schema *value_schema_manager::get_value_schema(uint32_t meta_cf_data_versi
} else {
auto schema = get_value_schema(meta_cf_data_version);
if (nullptr == schema) {
- dassert_f(false, "data version({}) in meta cf is not supported", meta_cf_data_version);
+ CHECK(false, "data version({}) in meta cf is not supported", meta_cf_data_version);
}
Review Comment:
```c++
CHECK(schema != nullptr, "data version({}) in meta cf is not supported", meta_cf_data_version);
```
##########
src/common/replication_common.cpp:
##########
@@ -160,7 +160,7 @@ void replication_options::initialize()
}
if (data_dirs.empty()) {
- dassert_f(false, "no replica data dir found, maybe not set or excluded by black list");
+ CHECK(false, "no replica data dir found, maybe not set or excluded by black list");
}
Review Comment:
```c++
CHECK(!data_dirs.empty(), "no replica data dir found, maybe not set or excluded by black list");
```
##########
src/block_service/fds/fds_service.cpp:
##########
@@ -412,14 +412,14 @@ error_code fds_file_object::get_file_meta()
fds_service::FILE_LENGTH_CUSTOM_KEY.c_str(),
_fds_path.c_str());
bool valid = dsn::buf2uint64(iter->second, _size);
- dassert_f(valid, "error to get file size");
+ CHECK(valid, "error to get file size");
// get md5 key
iter = meta.find(fds_service::FILE_MD5_KEY);
dassert_f(iter != meta.end(),
Review Comment:
```suggestion
CHECK(iter != meta.end(),
```
##########
src/common/replication_common.cpp:
##########
@@ -581,7 +581,7 @@ replication_options::get_data_dirs_in_black_list(const std::string &fname,
LOG_INFO_F("data_dirs_black_list_file[{}] found, apply it", fname);
std::ifstream file(fname);
if (!file) {
- dassert_f(false, "open data_dirs_black_list_file failed: {}", fname);
+ CHECK(false, "open data_dirs_black_list_file failed: {}", fname);
}
Review Comment:
```c++
CHECK(file, "open data_dirs_black_list_file failed: {}", fname);
```
##########
src/meta/backup_engine.cpp:
##########
@@ -103,9 +103,7 @@ error_code backup_engine::write_backup_file(const std::string &file_name,
LOG_INFO_F("create file {} failed", file_name);
return err;
}
- dassert_f(remote_file != nullptr,
- "create file {} succeed, but can't get handle",
- create_file_req.file_name);
+ CHECK(remote_file, "create file {} succeed, but can't get handle", create_file_req.file_name);
Review Comment:
```suggestion
CHECK(remote_file != nullptr, "create file {} succeed, but can't get handle", create_file_req.file_name);
```
##########
src/server/available_detector.cpp:
##########
@@ -79,13 +79,13 @@ available_detector::available_detector()
"available detect timeout");
// initialize the _client.
if (!pegasus_client_factory::initialize(nullptr)) {
- dassert(false, "Initialize the pegasus client failed");
+ CHECK(false, "Initialize the pegasus client failed");
}
_client = pegasus_client_factory::get_client(_cluster_name.c_str(), _app_name.c_str());
- dassert(_client != nullptr, "Initialize the _client failed");
+ CHECK(_client, "Initialize the _client failed");
_result_writer = dsn::make_unique<result_writer>(_client);
_ddl_client.reset(new replication_ddl_client(_meta_list));
- dassert(_ddl_client != nullptr, "Initialize the _ddl_client failed");
+ CHECK(_ddl_client, "Initialize the _ddl_client failed");
Review Comment:
```suggestion
CHECK(_ddl_client != nullptr, "Initialize the _ddl_client failed");
```
##########
src/redis_protocol/proxy_lib/proxy_layer.cpp:
##########
@@ -105,7 +106,7 @@ void proxy_stub::remove_session(dsn::rpc_address remote_address)
proxy_session::proxy_session(proxy_stub *op, dsn::message_ex *first_msg)
: _stub(op), _is_session_reset(false), _backup_one_request(first_msg)
{
- dassert(first_msg != nullptr, "null msg when create session");
+ CHECK(first_msg, "null msg when create session");
Review Comment:
```suggestion
CHECK(first_msg != nullptr, "null msg when create session");
```
##########
src/server/pegasus_server_impl.cpp:
##########
@@ -114,7 +114,7 @@ void pegasus_server_impl::parse_checkpoints()
pegasus_server_impl::~pegasus_server_impl()
{
if (_is_open) {
- dassert(_db != nullptr, "");
+ CHECK(_db, "");
Review Comment:
```suggestion
CHECK(_db != nullptr, "");
```
##########
src/server/pegasus_server_impl.cpp:
##########
@@ -258,15 +258,15 @@ int pegasus_server_impl::on_batched_write_requests(int64_t decree,
dsn::message_ex **requests,
int count)
{
- dassert(_is_open, "");
- dassert(requests != nullptr, "");
+ CHECK(_is_open, "");
+ CHECK(requests, "");
Review Comment:
```suggestion
CHECK(requests != nullptr, "");
```
##########
src/server/pegasus_server_impl.cpp:
##########
@@ -1761,15 +1761,15 @@ dsn::error_code pegasus_server_impl::start(int argc, char **argv)
void pegasus_server_impl::cancel_background_work(bool wait)
{
if (_is_open) {
- dassert(_db != nullptr, "");
+ CHECK(_db, "");
rocksdb::CancelAllBackgroundWork(_db, wait);
}
}
::dsn::error_code pegasus_server_impl::stop(bool clear_state)
{
if (!_is_open) {
- dassert(_db == nullptr, "");
+ dassert(!_db, "");
Review Comment:
```suggestion
dassert(_db == nullptr, "");
```
##########
src/test/bench_test/benchmark.cpp:
##########
@@ -60,7 +60,7 @@ void benchmark::run_benchmark(int thread_count, operation_type op_type)
{
// get method by operation type
bench_method method = _operation_method[op_type];
- dassert_f(method, "");
+ CHECK(method, "");
Review Comment:
```suggestion
CHECK(method != nullptr, "");
```
##########
src/test/kill_test/process_kill_testor.cpp:
##########
@@ -53,7 +53,7 @@ process_kill_testor::process_kill_testor(const char *config_file) : kill_testor(
dsn_config_get_value_string(section, "killer_handler", "", "killer handler");
dassert(killer_name.size() > 0, "");
_killer_handler.reset(killer_handler::new_handler(killer_name.c_str()));
- dassert(_killer_handler.get() != nullptr, "invalid killer_name(%s)", killer_name.c_str());
+ CHECK(_killer_handler.get(), "invalid killer_name({})", killer_name);
Review Comment:
```suggestion
CHECK(_killer_handler, "invalid killer_name({})", killer_name);
```
##########
src/test/pressure_test/main.cpp:
##########
@@ -253,20 +254,20 @@ int main(int argc, const char **argv)
(int32_t)dsn_config_get_value_uint64("pressureclient", "value_len", 64, "value length");
dassert(qps > 0, "qps must GT 0, but qps(%d)", qps);
- dassert(!op_name.empty(), "must assign operation name");
+ CHECK(!op_name.empty(), "must assign operation name");
LOG_INFO("pressureclient %s qps = %d", op_name.c_str(), qps);
pg_client = pegasus_client_factory::get_client(cluster_name.c_str(), app_name.c_str());
- dassert(pg_client != nullptr, "initialize pg_client failed");
+ CHECK(pg_client, "initialize pg_client failed");
Review Comment:
```suggestion
CHECK(pg_client != nullptr, "initialize pg_client failed");
```
##########
src/server/info_collector.cpp:
##########
@@ -69,13 +69,13 @@ info_collector::info_collector()
_usage_stat_app = dsn_config_get_value_string(
"pegasus.collector", "usage_stat_app", "", "app for recording usage statistics");
- dassert(!_usage_stat_app.empty(), "");
+ CHECK(!_usage_stat_app.empty(), "");
// initialize the _client.
if (!pegasus_client_factory::initialize(nullptr)) {
- dassert(false, "Initialize the pegasus client failed");
+ CHECK(false, "Initialize the pegasus client failed");
}
_client = pegasus_client_factory::get_client(_cluster_name.c_str(), _usage_stat_app.c_str());
- dassert(_client != nullptr, "Initialize the client failed");
+ CHECK(_client, "Initialize the client failed");
Review Comment:
```suggestion
CHECK(_client != nullptr, "Initialize the client failed");
```
##########
src/replica/replication_app_base.cpp:
##########
@@ -75,7 +75,7 @@ error_code write_blob_to_file(const std::string &file, const blob &data)
sz = s;
},
0);
- dassert_f(tsk, "create file::write task failed");
+ CHECK(tsk, "create file::write task failed");
Review Comment:
```suggestion
CHECK(tsk != nullptr, "create file::write task failed");
```
##########
src/server/available_detector.cpp:
##########
@@ -79,13 +79,13 @@ available_detector::available_detector()
"available detect timeout");
// initialize the _client.
if (!pegasus_client_factory::initialize(nullptr)) {
- dassert(false, "Initialize the pegasus client failed");
+ CHECK(false, "Initialize the pegasus client failed");
}
_client = pegasus_client_factory::get_client(_cluster_name.c_str(), _app_name.c_str());
- dassert(_client != nullptr, "Initialize the _client failed");
+ CHECK(_client, "Initialize the _client failed");
Review Comment:
```suggestion
CHECK(_client != nullptr, "Initialize the _client failed");
```
##########
src/geo/lib/geo_client.cpp:
##########
@@ -67,13 +67,13 @@ geo_client::geo_client(const char *config_file,
const char *geo_app_name)
{
bool ok = pegasus_client_factory::initialize(config_file);
- dassert(ok, "init pegasus client factory failed");
+ CHECK(ok, "init pegasus client factory failed");
_common_data_client = pegasus_client_factory::get_client(cluster_name, common_app_name);
- dassert(_common_data_client != nullptr, "init pegasus _common_data_client failed");
+ CHECK(_common_data_client, "init pegasus _common_data_client failed");
Review Comment:
```suggestion
CHECK(_common_data_client != nullptr, "init pegasus _common_data_client failed");
```
##########
src/geo/lib/geo_client.cpp:
##########
@@ -67,13 +67,13 @@ geo_client::geo_client(const char *config_file,
const char *geo_app_name)
{
bool ok = pegasus_client_factory::initialize(config_file);
- dassert(ok, "init pegasus client factory failed");
+ CHECK(ok, "init pegasus client factory failed");
_common_data_client = pegasus_client_factory::get_client(cluster_name, common_app_name);
- dassert(_common_data_client != nullptr, "init pegasus _common_data_client failed");
+ CHECK(_common_data_client, "init pegasus _common_data_client failed");
_geo_data_client = pegasus_client_factory::get_client(cluster_name, geo_app_name);
- dassert(_geo_data_client != nullptr, "init pegasus _geo_data_client failed");
+ CHECK(_geo_data_client, "init pegasus _geo_data_client failed");
Review Comment:
```suggestion
CHECK(_geo_data_client != nullptr, "init pegasus _geo_data_client failed");
```
##########
src/server/pegasus_write_service_impl.h:
##########
@@ -674,7 +674,7 @@ class pegasus_write_service::impl : public dsn::replication::replica_base
}
}
default:
- dassert(false, "unsupported check type: %d", check_type);
+ CHECK(false, "unsupported check type: %d", check_type);
Review Comment:
```suggestion
CHECK(false, "unsupported check type: {}", check_type);
```
##########
src/server/pegasus_server_impl.cpp:
##########
@@ -1761,15 +1761,15 @@ dsn::error_code pegasus_server_impl::start(int argc, char **argv)
void pegasus_server_impl::cancel_background_work(bool wait)
{
if (_is_open) {
- dassert(_db != nullptr, "");
+ CHECK(_db, "");
Review Comment:
```suggestion
CHECK(_db != nullptr, "");
```
##########
src/shell/commands/debugger.cpp:
##########
@@ -90,7 +90,7 @@ bool mlog_dump(command_executor *e, shell_context *sc, arguments args)
int64_t decree, int64_t timestamp, dsn::message_ex **requests, int count) mutable {
for (int i = 0; i < count; ++i) {
dsn::message_ex *request = requests[i];
- dassert(request != nullptr, "");
+ CHECK(request, "");
Review Comment:
```suggestion
CHECK(request != nullptr, "");
```
##########
src/server/pegasus_server_write.cpp:
##########
@@ -84,7 +84,7 @@ int pegasus_server_write::on_batched_writes(dsn::message_ex **requests, int coun
_write_svc->batch_prepare(_decree);
for (int i = 0; i < count; ++i) {
- dassert(requests[i] != nullptr, "request[%d] is null", i);
+ CHECK(requests[i], "request[{}] is null", i);
Review Comment:
```suggestion
CHECK(requests[i] != nullptr, "request[{}] is null", i);
```
##########
src/test/bench_test/benchmark.cpp:
##########
@@ -33,7 +33,7 @@ benchmark::benchmark()
{
_client = pegasus_client_factory::get_client(config::instance().pegasus_cluster_name.c_str(),
config::instance().pegasus_app_name.c_str());
- dassert_f(_client, "");
+ CHECK(_client, "");
Review Comment:
```suggestion
CHECK(_client != nullptr, "");
```
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1205: refactor(macro): Use CHECK to replace dassert_f/dassert
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on code in PR #1205:
URL: https://github.com/apache/incubator-pegasus/pull/1205#discussion_r1006431104
##########
src/redis_protocol/proxy_lib/redis_parser.cpp:
##########
@@ -369,7 +368,7 @@ void redis_parser::reply_all_ready()
std::vector<dsn::message_ex *> ready_responses;
fetch_and_dequeue_messages(ready_responses, true);
for (dsn::message_ex *m : ready_responses) {
- dassert(m != nullptr, "");
+ CHECK(m, "");
Review Comment:
Added a new macro `CHECK_NOTNULL` for pointers, similar to [glog CHECK_NOTNULL](https://github.com/google/glog/blob/b70ea80433c2a8f20b832be97b90f1f82b0d29e9/src/glog/logging.h.in#L945)
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1205: refactor(macro): Use CHECK to replace dassert_f/dassert
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on code in PR #1205:
URL: https://github.com/apache/incubator-pegasus/pull/1205#discussion_r1006380970
##########
src/redis_protocol/proxy_lib/redis_parser.cpp:
##########
@@ -369,7 +368,7 @@ void redis_parser::reply_all_ready()
std::vector<dsn::message_ex *> ready_responses;
fetch_and_dequeue_messages(ready_responses, true);
for (dsn::message_ex *m : ready_responses) {
- dassert(m != nullptr, "");
+ CHECK(m, "");
Review Comment:
Both are OK.
But in the future refactor, we are going to introduce `CHECK_EQ` and `CHECK_NE`, it's a bit of duplicate and strange if we judge the pointer like `CHECK_NE(pointer, nullptr)`. I also consulted some other projects' habit, both styles are OK.
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1205: refactor(macro): Use CHECK to replace dassert_f/dassert
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on code in PR #1205:
URL: https://github.com/apache/incubator-pegasus/pull/1205#discussion_r1006388211
##########
src/block_service/fds/fds_service.cpp:
##########
@@ -412,14 +412,14 @@ error_code fds_file_object::get_file_meta()
fds_service::FILE_LENGTH_CUSTOM_KEY.c_str(),
_fds_path.c_str());
bool valid = dsn::buf2uint64(iter->second, _size);
- dassert_f(valid, "error to get file size");
+ CHECK(valid, "error to get file size");
// get md5 key
iter = meta.find(fds_service::FILE_MD5_KEY);
dassert_f(iter != meta.end(),
Review Comment:
Will be refactored by using `CHECK_NE`
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1205: refactor(macro): Use CHECK to replace dassert_f/dassert
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on code in PR #1205:
URL: https://github.com/apache/incubator-pegasus/pull/1205#discussion_r1006431104
##########
src/redis_protocol/proxy_lib/redis_parser.cpp:
##########
@@ -369,7 +368,7 @@ void redis_parser::reply_all_ready()
std::vector<dsn::message_ex *> ready_responses;
fetch_and_dequeue_messages(ready_responses, true);
for (dsn::message_ex *m : ready_responses) {
- dassert(m != nullptr, "");
+ CHECK(m, "");
Review Comment:
Added a new macro `CHECK_NOTNULL` for pointers
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] empiredan merged pull request #1205: refactor(macro): Use CHECK to replace dassert_f/dassert
Posted by GitBox <gi...@apache.org>.
empiredan merged PR #1205:
URL: https://github.com/apache/incubator-pegasus/pull/1205
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1205: refactor(macro): Use CHECK to replace dassert_f/dassert
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on code in PR #1205:
URL: https://github.com/apache/incubator-pegasus/pull/1205#discussion_r1006388211
##########
src/block_service/fds/fds_service.cpp:
##########
@@ -412,14 +412,14 @@ error_code fds_file_object::get_file_meta()
fds_service::FILE_LENGTH_CUSTOM_KEY.c_str(),
_fds_path.c_str());
bool valid = dsn::buf2uint64(iter->second, _size);
- dassert_f(valid, "error to get file size");
+ CHECK(valid, "error to get file size");
// get md5 key
iter = meta.find(fds_service::FILE_MD5_KEY);
dassert_f(iter != meta.end(),
Review Comment:
Will refactored by using `CHECK_NE`
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] levy5307 commented on a diff in pull request #1205: refactor(macro): Use CHECK to replace dassert_f/dassert
Posted by GitBox <gi...@apache.org>.
levy5307 commented on code in PR #1205:
URL: https://github.com/apache/incubator-pegasus/pull/1205#discussion_r1006352666
##########
src/redis_protocol/proxy_lib/redis_parser.cpp:
##########
@@ -369,7 +368,7 @@ void redis_parser::reply_all_ready()
std::vector<dsn::message_ex *> ready_responses;
fetch_and_dequeue_messages(ready_responses, true);
for (dsn::message_ex *m : ready_responses) {
- dassert(m != nullptr, "");
+ CHECK(m, "");
Review Comment:
It's better to user `m != nullptr` for pointer
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org