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 2023/01/19 04:25:16 UTC
[GitHub] [incubator-pegasus] empiredan commented on a diff in pull request #1319: refactor(log): use LOG_WARNING_F instead of LOG_WARNING (3/3)
empiredan commented on code in PR #1319:
URL: https://github.com/apache/incubator-pegasus/pull/1319#discussion_r1080750931
##########
src/common/storage_serverlet.h:
##########
@@ -129,10 +129,10 @@ class storage_serverlet
if (ptr != nullptr) {
(*ptr)(static_cast<T *>(this), request);
} else {
- LOG_WARNING("recv message with unhandled rpc name %s from %s, trace_id = %016" PRIx64,
- t.to_string(),
- request->header->from_address.to_string(),
- request->header->trace_id);
+ LOG_WARNING_F("recv message with unhandled rpc name {} from {}, trace_id = {:#018x} ",
+ t,
+ request->header->from_address.to_string(),
Review Comment:
```suggestion
request->header->from_address,
```
##########
src/client/partition_resolver_simple.cpp:
##########
@@ -276,11 +276,10 @@ void partition_resolver_simple::query_config_reply(error_code err,
if (_app_partition_count != -1 && _app_partition_count != resp.partition_count &&
_app_partition_count * 2 != resp.partition_count &&
_app_partition_count != resp.partition_count * 2) {
- LOG_WARNING_F(
- "partition count is changed (mostly the app was removed and created with "
- "the same name), local Vs remote: %u vs %u ",
- _app_partition_count,
- resp.partition_count);
+ LOG_WARNING_F("partition count is changed (mostly the app was removed and created "
+ "with the same name), local Vs remote: {} vs {} ",
Review Comment:
```suggestion
"with the same name), local vs remote: {} vs {} ",
```
##########
src/redis_protocol/proxy_lib/redis_parser.cpp:
##########
@@ -918,10 +918,10 @@ void redis_parser::counter_internal(message_entry &entry)
}
if (!dsn::buf2int64(entry.request.sub_requests[2].data, increment)) {
LOG_WARNING_F("{}: command {} seqid({}) with invalid 'increment': {}",
- _remote_address.to_string(),
+ _remote_address,
command,
entry.sequence_id,
- entry.request.sub_requests[2].data.to_string());
+ entry.request.sub_requests[2].data);
Review Comment:
`blob` does not overload `operator<<` (since blob might be binary), thus I think we have to keep `.to_string()` here:
```suggestion
entry.request.sub_requests[2].data.to_string());
```
##########
src/redis_protocol/proxy_lib/redis_parser.cpp:
##########
@@ -938,15 +938,15 @@ void redis_parser::counter_internal(message_entry &entry)
::dsn::error_code ec, dsn::message_ex *, dsn::message_ex *response) {
if (_is_session_reset.load(std::memory_order_acquire)) {
LOG_WARNING_F("{}: command {} seqid({}) got reply, but session has reset",
- _remote_address.to_string(),
+ _remote_address,
command,
entry.sequence_id);
return;
}
if (::dsn::ERR_OK != ec) {
LOG_WARNING_F("{}: command {} seqid({}) got reply with error = {}",
- _remote_address.to_string(),
+ _remote_address,
command,
entry.sequence_id,
ec.to_string());
Review Comment:
```suggestion
ec);
```
##########
src/runtime/rpc/rpc_engine.cpp:
##########
@@ -502,10 +501,10 @@ error_code rpc_engine::start(const service_app_spec &aspec)
(*pnets)[sp.second.channel].reset(net);
- LOG_WARNING("[%s] network server started at port %u, channel = %s, ...",
- node()->full_name(),
- (uint32_t)(port),
- sp.second.channel.to_string());
+ LOG_WARNING_F("[{}] network server started at port {}, channel = {}, ...",
+ node()->full_name(),
+ (uint32_t)(port),
Review Comment:
I think the number for int-typed port will not be negative, thus no need to cast again:
```suggestion
port,
```
##########
src/runtime/rpc/rpc_engine.cpp:
##########
@@ -534,9 +533,8 @@ bool rpc_engine::unregister_rpc_handler(dsn::task_code rpc_code)
void rpc_engine::on_recv_request(network *net, message_ex *msg, int delay_ms)
{
if (!_is_serving) {
- LOG_WARNING(
- "recv message with rpc name %s from %s when rpc engine is not serving, trace_id = "
- "%" PRIu64,
+ LOG_WARNING_F(
+ "recv message with rpc name {} from {} when rpc engine is not serving, trace_id = {}",
msg->header->rpc_name,
msg->header->from_address.to_string(),
Review Comment:
```suggestion
msg->header->from_address,
```
##########
src/nfs/nfs_client_impl.cpp:
##########
@@ -334,13 +334,13 @@ void nfs_client_impl::end_copy(::dsn::error_code err,
if (!fc->user_req->is_finished) {
if (reqc->retry_count > 0) {
- LOG_WARNING("{nfs_service} remote copy failed, source = %s, dir = %s, file = %s, "
- "err = %s, retry_count = %d",
- fc->user_req->file_size_req.source.to_string(),
- fc->user_req->file_size_req.source_dir.c_str(),
- fc->file_name.c_str(),
- err.to_string(),
- reqc->retry_count);
+ LOG_WARNING_F("{nfs_service} remote copy failed, source = {}, dir = {}, file = {}, "
Review Comment:
`{nfs_service}` will be recognized as a placeholder and letters in it will be parsed as options, thus raising `fmt::v5::format_error` which then leads to core dump. Therefore we can use `[]` instead:
```suggestion
LOG_WARNING_F("[nfs_service] remote copy failed, source = {}, dir = {}, file = {}, "
```
##########
src/runtime/rpc/rpc_engine.cpp:
##########
@@ -584,21 +582,21 @@ void rpc_engine::on_recv_request(network *net, message_ex *msg, int delay_ms)
tsk->release_ref();
}
} else {
- LOG_WARNING("recv message with unhandled rpc name %s from %s, trace_id = %016" PRIx64,
- msg->header->rpc_name,
- msg->header->from_address.to_string(),
- msg->header->trace_id);
+ LOG_WARNING_F("recv message with unhandled rpc name {} from {}, trace_id = {:#018x}",
+ msg->header->rpc_name,
+ msg->header->from_address.to_string(),
Review Comment:
```suggestion
msg->header->from_address,
```
##########
src/runtime/task/task.cpp:
##########
@@ -251,11 +251,11 @@ static void check_wait_task(task *waitee)
task::get_current_worker()->pool_spec().worker_count > 1)
return;
- LOG_WARNING("task %s waits for another task %s sharing the same thread pool "
- "- will lead to deadlocks easily (e.g., when worker_count = 1 or when the pool "
- "is partitioned)",
- task::get_current_task()->spec().code.to_string(),
- waitee->spec().code.to_string());
+ LOG_WARNING_F("task {} waits for another task {} sharing the same thread pool "
+ "- will lead to deadlocks easily (e.g., when worker_count = 1 or when the pool "
+ "is partitioned)",
+ task::get_current_task()->spec().code,
Review Comment:
```suggestion
task::get_current_task()->code(),
```
##########
src/runtime/rpc/rpc_engine.cpp:
##########
@@ -584,21 +582,21 @@ void rpc_engine::on_recv_request(network *net, message_ex *msg, int delay_ms)
tsk->release_ref();
}
} else {
- LOG_WARNING("recv message with unhandled rpc name %s from %s, trace_id = %016" PRIx64,
- msg->header->rpc_name,
- msg->header->from_address.to_string(),
- msg->header->trace_id);
+ LOG_WARNING_F("recv message with unhandled rpc name {} from {}, trace_id = {:#018x}",
+ msg->header->rpc_name,
+ msg->header->from_address.to_string(),
+ msg->header->trace_id);
CHECK_EQ_MSG(msg->get_count(), 0, "request should not be referenced by anybody so far");
msg->add_ref();
dsn_rpc_reply(msg->create_response(), ::dsn::ERR_HANDLER_NOT_FOUND);
msg->release_ref();
}
} else {
- LOG_WARNING("recv message with unknown rpc name %s from %s, trace_id = %016" PRIx64,
- msg->header->rpc_name,
- msg->header->from_address.to_string(),
- msg->header->trace_id);
+ LOG_WARNING_F("recv message with unknown rpc name {} from {}, trace_id = {:#018x}",
+ msg->header->rpc_name,
+ msg->header->from_address.to_string(),
Review Comment:
```suggestion
msg->header->from_address,
```
##########
src/runtime/task/task_worker.cpp:
##########
@@ -103,7 +103,7 @@ void task_worker::set_name(const char *name)
#endif // defined(__linux__)
// We expect EPERM failures in sandboxed processes, just ignore those.
if (err < 0 && errno != EPERM) {
- LOG_WARNING("Fail to set pthread name. err = %d", err);
+ LOG_WARNING_F("Fail to set pthread name. err = {}", err);
Review Comment:
```suggestion
LOG_WARNING_F("Fail to set pthread name: err = {}, msg = {}", err, utils::safe_strerror(errno));
```
##########
src/runtime/task/task_worker.cpp:
##########
@@ -159,7 +159,7 @@ void task_worker::set_affinity(uint64_t affinity)
err = pthread_setaffinity_np(pthread_self(), sizeof(cpuset), &cpuset);
if (err != 0) {
- LOG_WARNING("Fail to set thread affinity. err = %d", err);
+ LOG_WARNING_F("Fail to set thread affinity. err = {}", err);
Review Comment:
```suggestion
LOG_WARNING_F("Fail to set thread affinity. err = {}, msg = {}", err, utils::safe_strerror(errno));
```
##########
src/runtime/task/task_worker.cpp:
##########
@@ -128,7 +128,7 @@ void task_worker::set_priority(worker_priority_t pri)
succ = false;
}
if (!succ) {
- LOG_WARNING("You may need priviledge to set thread priority. errno = %d", errno);
+ LOG_WARNING_F("You may need priviledge to set thread priority. errno = {}", errno);
Review Comment:
```suggestion
LOG_WARNING_F("You may need priviledge to set thread priority: errno = {}, msg = {}", errno, utils::safe_strerror(errno));
```
##########
src/server/pegasus_server_impl.cpp:
##########
@@ -438,24 +438,24 @@ void pegasus_server_impl::on_multi_get(multi_get_rpc rpc)
if (c > 0 || (c == 0 && (!start_inclusive || !stop_inclusive))) {
// empty sort key range
if (_verbose_log) {
- LOG_WARNING(
- "%s: empty sort key range for multi_get from %s: hash_key = \"%s\", "
- "start_sort_key = \"%s\" (%s), stop_sort_key = \"%s\" (%s), "
- "sort_key_filter_type = %s, sort_key_filter_pattern = \"%s\", "
- "final_start = \"%s\" (%s), final_stop = \"%s\" (%s)",
+ LOG_WARNING_F(
Review Comment:
Use `LOG_WARNING_PREFIX` instead:
```suggestion
LOG_WARNING_PREFIX(
```
##########
src/runtime/task/task.cpp:
##########
@@ -251,11 +251,11 @@ static void check_wait_task(task *waitee)
task::get_current_worker()->pool_spec().worker_count > 1)
return;
- LOG_WARNING("task %s waits for another task %s sharing the same thread pool "
- "- will lead to deadlocks easily (e.g., when worker_count = 1 or when the pool "
- "is partitioned)",
- task::get_current_task()->spec().code.to_string(),
- waitee->spec().code.to_string());
+ LOG_WARNING_F("task {} waits for another task {} sharing the same thread pool "
+ "- will lead to deadlocks easily (e.g., when worker_count = 1 or when the pool "
+ "is partitioned)",
+ task::get_current_task()->spec().code,
+ waitee->spec().code);
Review Comment:
```suggestion
waitee->code());
```
##########
src/server/pegasus_server_impl.cpp:
##########
@@ -438,24 +438,24 @@ void pegasus_server_impl::on_multi_get(multi_get_rpc rpc)
if (c > 0 || (c == 0 && (!start_inclusive || !stop_inclusive))) {
// empty sort key range
if (_verbose_log) {
- LOG_WARNING(
- "%s: empty sort key range for multi_get from %s: hash_key = \"%s\", "
- "start_sort_key = \"%s\" (%s), stop_sort_key = \"%s\" (%s), "
- "sort_key_filter_type = %s, sort_key_filter_pattern = \"%s\", "
- "final_start = \"%s\" (%s), final_stop = \"%s\" (%s)",
+ LOG_WARNING_F(
+ "{}: empty sort key range for multi_get from {}: hash_key = \"{}\", "
Review Comment:
```suggestion
"empty sort key range for multi_get from {}: hash_key = \"{}\", "
```
##########
src/server/pegasus_server_impl.cpp:
##########
@@ -438,24 +438,24 @@ void pegasus_server_impl::on_multi_get(multi_get_rpc rpc)
if (c > 0 || (c == 0 && (!start_inclusive || !stop_inclusive))) {
// empty sort key range
if (_verbose_log) {
- LOG_WARNING(
- "%s: empty sort key range for multi_get from %s: hash_key = \"%s\", "
- "start_sort_key = \"%s\" (%s), stop_sort_key = \"%s\" (%s), "
- "sort_key_filter_type = %s, sort_key_filter_pattern = \"%s\", "
- "final_start = \"%s\" (%s), final_stop = \"%s\" (%s)",
+ LOG_WARNING_F(
+ "{}: empty sort key range for multi_get from {}: hash_key = \"{}\", "
+ "start_sort_key = \"{}\" ({}), stop_sort_key = \"{}\" ({}), "
+ "sort_key_filter_type = {}, sort_key_filter_pattern = \"{}\", final_start = "
+ "\"{}\" ({}), final_stop = \"{}\" ({})",
replica_name(),
Review Comment:
```suggestion
```
--
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