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/18 08:18:24 UTC
[GitHub] [incubator-pegasus] WHBANG opened a new pull request, #1318: refactor(log): use LOG_ERROR_F instead of LOG_ERROR (1/2)
WHBANG opened a new pull request, #1318:
URL: https://github.com/apache/incubator-pegasus/pull/1318
### What problem does this PR solve? <!--add issue link with summary if exists-->
https://github.com/apache/incubator-pegasus/issues/1305
This patch aim to use LOG_ERROR_F instead of LOG_ERROR. Includes:
Use LOG_ERROR_F instead of LOG_ERROR in replica module.
--
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 #1318: refactor(log): use LOG_ERROR_F instead of LOG_ERROR (1/2)
Posted by GitBox <gi...@apache.org>.
empiredan commented on code in PR #1318:
URL: https://github.com/apache/incubator-pegasus/pull/1318#discussion_r1073233822
##########
src/replica/replica_stub.cpp:
##########
@@ -1633,15 +1631,15 @@ void replica_stub::response_client(gpid id,
_counter_recent_read_fail_count->increment();
else
_counter_recent_write_fail_count->increment();
- LOG_ERROR("%s@%s: %s fail: client = %s, code = %s, timeout = %d, status = %s, error = %s",
- id.to_string(),
- _primary_address_str,
- is_read ? "read" : "write",
- request == nullptr ? "null" : request->header->from_address.to_string(),
- request == nullptr ? "null" : request->header->rpc_name,
- request == nullptr ? 0 : request->header->client.timeout_ms,
- enum_to_string(status),
- error.to_string());
+ LOG_ERROR_F("{}@{}: {} fail: client = {}, code = {}, timeout = {}, status = {}, error = {}",
+ id,
+ _primary_address_str,
+ is_read ? "read" : "write",
+ request == nullptr ? "null" : request->header->from_address,
Review Comment:
Since `const char *` and `rpc_address` have different types, and cannot convert to each other:
```suggestion
request == nullptr ? "null" : request->header->from_address.to_string(),
```
##########
src/replica/replica_restore.cpp:
##########
@@ -44,7 +44,7 @@ bool replica::remove_useless_file_under_chkpt(const std::string &chkpt_dir,
// filename --> file_path such as: file --> ***/***/file
std::map<std::string, std::string> name_to_filepath;
if (!::dsn::utils::filesystem::get_subfiles(chkpt_dir, sub_files, false)) {
- LOG_ERROR("%s: get subfile of dir(%s) failed", name(), chkpt_dir.c_str());
+ LOG_ERROR_PREFIX("get subfile of dir(%s) failed", chkpt_dir);
Review Comment:
```suggestion
LOG_ERROR_PREFIX("get subfile of dir({}) failed", chkpt_dir);
```
##########
src/replica/log_file.cpp:
##########
@@ -203,27 +202,27 @@ error_code log_file::read_next_log_block(/*out*/ ::dsn::blob &bb)
// if read_count is 0, then we meet the end of file
err = (bb.length() == 0 ? ERR_HANDLE_EOF : ERR_INCOMPLETE_DATA);
} else {
- LOG_ERROR("read data block header failed, size = %d vs %d, err = %s",
- bb.length(),
- (int)sizeof(log_block_header),
- err.to_string());
+ LOG_ERROR_F("read data block header failed, size = {} vs {}, err = {}",
+ bb.length(),
+ (int)sizeof(log_block_header),
Review Comment:
```suggestion
sizeof(log_block_header),
```
##########
src/replica/replica_2pc.cpp:
##########
@@ -524,10 +518,8 @@ void replica::on_append_log_completed(mutation_ptr &mu, error_code err, size_t s
if (err == ERR_OK) {
mu->set_logged();
} else {
- LOG_ERROR("%s: append shared log failed for mutation %s, err = %s",
- name(),
- mu->name(),
- err.to_string());
+ LOG_ERROR_PREFIX(
+ "append shared log failed for mutation {}, err = {}", mu->name(), err.to_string());
Review Comment:
```suggestion
"append shared log failed for mutation {}, err = {}", mu->name(), err);
```
##########
src/replica/log_file.cpp:
##########
@@ -203,27 +202,27 @@ error_code log_file::read_next_log_block(/*out*/ ::dsn::blob &bb)
// if read_count is 0, then we meet the end of file
err = (bb.length() == 0 ? ERR_HANDLE_EOF : ERR_INCOMPLETE_DATA);
} else {
- LOG_ERROR("read data block header failed, size = %d vs %d, err = %s",
- bb.length(),
- (int)sizeof(log_block_header),
- err.to_string());
+ LOG_ERROR_F("read data block header failed, size = {} vs {}, err = {}",
+ bb.length(),
+ (int)sizeof(log_block_header),
+ err);
}
return err;
}
log_block_header hdr = *reinterpret_cast<const log_block_header *>(bb.data());
if (hdr.magic != 0xdeadbeef) {
- LOG_ERROR("invalid data header magic: 0x%x", hdr.magic);
+ LOG_ERROR_F("invalid data header magic: {:#x}", static_cast<uint32_t>(hdr.magic));
return ERR_INVALID_DATA;
}
err = _stream->read_next(hdr.length, bb);
if (err != ERR_OK || hdr.length != bb.length()) {
- LOG_ERROR("read data block body failed, size = %d vs %d, err = %s",
- bb.length(),
- (int)hdr.length,
- err.to_string());
+ LOG_ERROR_F("read data block body failed, size = {} vs {}, err = {}",
+ bb.length(),
+ (int)hdr.length,
Review Comment:
```suggestion
hdr.length,
```
--
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] WHBANG commented on a diff in pull request #1318: refactor(log): use LOG_ERROR_F instead of LOG_ERROR (1/2)
Posted by GitBox <gi...@apache.org>.
WHBANG commented on code in PR #1318:
URL: https://github.com/apache/incubator-pegasus/pull/1318#discussion_r1073284465
##########
src/replica/replica_2pc.cpp:
##########
@@ -524,10 +518,8 @@ void replica::on_append_log_completed(mutation_ptr &mu, error_code err, size_t s
if (err == ERR_OK) {
mu->set_logged();
} else {
- LOG_ERROR("%s: append shared log failed for mutation %s, err = %s",
- name(),
- mu->name(),
- err.to_string());
+ LOG_ERROR_PREFIX(
+ "append shared log failed for mutation {}, err = {}", mu->name(), err.to_string());
Review Comment:
done
--
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] WHBANG commented on a diff in pull request #1318: refactor(log): use LOG_ERROR_F instead of LOG_ERROR (1/2)
Posted by GitBox <gi...@apache.org>.
WHBANG commented on code in PR #1318:
URL: https://github.com/apache/incubator-pegasus/pull/1318#discussion_r1073284131
##########
src/replica/replica_restore.cpp:
##########
@@ -44,7 +44,7 @@ bool replica::remove_useless_file_under_chkpt(const std::string &chkpt_dir,
// filename --> file_path such as: file --> ***/***/file
std::map<std::string, std::string> name_to_filepath;
if (!::dsn::utils::filesystem::get_subfiles(chkpt_dir, sub_files, false)) {
- LOG_ERROR("%s: get subfile of dir(%s) failed", name(), chkpt_dir.c_str());
+ LOG_ERROR_PREFIX("get subfile of dir(%s) failed", chkpt_dir);
Review Comment:
done
##########
src/replica/log_file.cpp:
##########
@@ -203,27 +202,27 @@ error_code log_file::read_next_log_block(/*out*/ ::dsn::blob &bb)
// if read_count is 0, then we meet the end of file
err = (bb.length() == 0 ? ERR_HANDLE_EOF : ERR_INCOMPLETE_DATA);
} else {
- LOG_ERROR("read data block header failed, size = %d vs %d, err = %s",
- bb.length(),
- (int)sizeof(log_block_header),
- err.to_string());
+ LOG_ERROR_F("read data block header failed, size = {} vs {}, err = {}",
+ bb.length(),
+ (int)sizeof(log_block_header),
Review Comment:
done
##########
src/replica/log_file.cpp:
##########
@@ -203,27 +202,27 @@ error_code log_file::read_next_log_block(/*out*/ ::dsn::blob &bb)
// if read_count is 0, then we meet the end of file
err = (bb.length() == 0 ? ERR_HANDLE_EOF : ERR_INCOMPLETE_DATA);
} else {
- LOG_ERROR("read data block header failed, size = %d vs %d, err = %s",
- bb.length(),
- (int)sizeof(log_block_header),
- err.to_string());
+ LOG_ERROR_F("read data block header failed, size = {} vs {}, err = {}",
+ bb.length(),
+ (int)sizeof(log_block_header),
+ err);
}
return err;
}
log_block_header hdr = *reinterpret_cast<const log_block_header *>(bb.data());
if (hdr.magic != 0xdeadbeef) {
- LOG_ERROR("invalid data header magic: 0x%x", hdr.magic);
+ LOG_ERROR_F("invalid data header magic: {:#x}", static_cast<uint32_t>(hdr.magic));
return ERR_INVALID_DATA;
}
err = _stream->read_next(hdr.length, bb);
if (err != ERR_OK || hdr.length != bb.length()) {
- LOG_ERROR("read data block body failed, size = %d vs %d, err = %s",
- bb.length(),
- (int)hdr.length,
- err.to_string());
+ LOG_ERROR_F("read data block body failed, size = {} vs {}, err = {}",
+ bb.length(),
+ (int)hdr.length,
Review Comment:
done
--
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 merged pull request #1318: refactor(log): use LOG_ERROR_F instead of LOG_ERROR (1/2)
Posted by GitBox <gi...@apache.org>.
acelyc111 merged PR #1318:
URL: https://github.com/apache/incubator-pegasus/pull/1318
--
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] WHBANG commented on a diff in pull request #1318: refactor(log): use LOG_ERROR_F instead of LOG_ERROR (1/2)
Posted by GitBox <gi...@apache.org>.
WHBANG commented on code in PR #1318:
URL: https://github.com/apache/incubator-pegasus/pull/1318#discussion_r1073279592
##########
src/replica/replica_stub.cpp:
##########
@@ -1633,15 +1631,15 @@ void replica_stub::response_client(gpid id,
_counter_recent_read_fail_count->increment();
else
_counter_recent_write_fail_count->increment();
- LOG_ERROR("%s@%s: %s fail: client = %s, code = %s, timeout = %d, status = %s, error = %s",
- id.to_string(),
- _primary_address_str,
- is_read ? "read" : "write",
- request == nullptr ? "null" : request->header->from_address.to_string(),
- request == nullptr ? "null" : request->header->rpc_name,
- request == nullptr ? 0 : request->header->client.timeout_ms,
- enum_to_string(status),
- error.to_string());
+ LOG_ERROR_F("{}@{}: {} fail: client = {}, code = {}, timeout = {}, status = {}, error = {}",
+ id,
+ _primary_address_str,
+ is_read ? "read" : "write",
+ request == nullptr ? "null" : request->header->from_address,
Review Comment:
done
--
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 #1318: refactor(log): use LOG_ERROR_F instead of LOG_ERROR (1/2)
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on code in PR #1318:
URL: https://github.com/apache/incubator-pegasus/pull/1318#discussion_r1073770884
##########
src/replica/replica_learn.cpp:
##########
@@ -1227,22 +1213,19 @@ error_code replica::handle_learning_succeeded_on_primary(::dsn::rpc_address node
{
auto it = _primary_states.learners.find(node);
if (it == _primary_states.learners.end()) {
- LOG_ERROR("%s: handle_learning_succeeded_on_primary[%016" PRIx64 "]: learner = %s, "
- "learner not found on primary, return ERR_LEARNER_NOT_FOUND",
- name(),
- learn_signature,
- node.to_string());
+ LOG_ERROR_PREFIX("handle_learning_succeeded_on_primary[{:#018x}]: learner = {}, learner "
+ "not found on primary, return ERR_LEARNER_NOT_FOUND",
+ learn_signature,
+ node);
return ERR_LEARNER_NOT_FOUND;
}
if (it->second.signature != (int64_t)learn_signature) {
- LOG_ERROR("%s: handle_learning_succeeded_on_primary[%016" PRIx64 "]: learner = %s, "
- "signature not matched, current signature on primary is [%016" PRIx64
- "], return ERR_INVALID_STATE",
- name(),
- learn_signature,
- node.to_string(),
- it->second.signature);
+ LOG_ERROR_F("handle_learning_succeeded_on_primary[{:#018x}]: learner = {}, signature not "
Review Comment:
Should it be `LOG_ERROR_PREFIX`?
--
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] WHBANG commented on a diff in pull request #1318: refactor(log): use LOG_ERROR_F instead of LOG_ERROR (1/2)
Posted by GitBox <gi...@apache.org>.
WHBANG commented on code in PR #1318:
URL: https://github.com/apache/incubator-pegasus/pull/1318#discussion_r1073850685
##########
src/replica/replica_learn.cpp:
##########
@@ -1227,22 +1213,19 @@ error_code replica::handle_learning_succeeded_on_primary(::dsn::rpc_address node
{
auto it = _primary_states.learners.find(node);
if (it == _primary_states.learners.end()) {
- LOG_ERROR("%s: handle_learning_succeeded_on_primary[%016" PRIx64 "]: learner = %s, "
- "learner not found on primary, return ERR_LEARNER_NOT_FOUND",
- name(),
- learn_signature,
- node.to_string());
+ LOG_ERROR_PREFIX("handle_learning_succeeded_on_primary[{:#018x}]: learner = {}, learner "
+ "not found on primary, return ERR_LEARNER_NOT_FOUND",
+ learn_signature,
+ node);
return ERR_LEARNER_NOT_FOUND;
}
if (it->second.signature != (int64_t)learn_signature) {
- LOG_ERROR("%s: handle_learning_succeeded_on_primary[%016" PRIx64 "]: learner = %s, "
- "signature not matched, current signature on primary is [%016" PRIx64
- "], return ERR_INVALID_STATE",
- name(),
- learn_signature,
- node.to_string(),
- it->second.signature);
+ LOG_ERROR_F("handle_learning_succeeded_on_primary[{:#018x}]: learner = {}, signature not "
Review Comment:
fixed
--
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