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/17 09:31:19 UTC
[GitHub] [incubator-pegasus] WHBANG opened a new pull request, #1315: refactor(log): use LOG_WARNING_F instead of LOG_WARNING (1/3)
WHBANG opened a new pull request, #1315:
URL: https://github.com/apache/incubator-pegasus/pull/1315
### What problem does this PR solve? <!--add issue link with summary if exists-->
#1305
This patch aim to use LOG_WARNING_F instead of LOG_WARNING. Includes:
Use LOG_WARNING_F instead of LOG_WARNING in replica module.
Use LOG_WARNING_PREFIX instaed of LOG_WARNING for class replica
### What is changed and how does it work?
### Checklist <!--REMOVE the items that are not applicable-->
##### Tests <!-- At least one of them must be included. -->
- Unit test
- Integration test
- Manual test (add detailed scripts or steps below)
- No code
##### Code changes
- Has exported function/method change
- Has exported variable/fields change
- Has interface methods change
- Has persistent data change
##### Side effects
- Possible performance regression
- Increased code complexity
- Breaking backward compatibility
##### Related changes
- Need to cherry-pick to the release branch
- Need to update the documentation
- Need to be included in the release note
--
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] neverchanje commented on a diff in pull request #1315: refactor(log): use LOG_WARNING_F instead of LOG_WARNING (1/3)
Posted by GitBox <gi...@apache.org>.
neverchanje commented on code in PR #1315:
URL: https://github.com/apache/incubator-pegasus/pull/1315#discussion_r1072495044
##########
src/replica/replica_check.cpp:
##########
@@ -160,12 +158,12 @@ void replica::on_group_check(const group_check_request &request,
if (request.config.ballot < get_ballot()) {
response.err = ERR_VERSION_OUTDATED;
- LOG_WARNING("%s: on_group_check reply %s", name(), response.err.to_string());
+ LOG_WARNING_PREFIX("on_group_check reply {}", response.err);
Review Comment:
```suggestion
LOG_WARNING_F("on_group_check reply {}", response.err);
```
##########
src/replica/replica_check.cpp:
##########
@@ -199,7 +197,7 @@ void replica::on_group_check(const group_check_request &request,
response.err = ERR_OK;
if (status() == partition_status::PS_ERROR) {
response.err = ERR_INVALID_STATE;
- LOG_WARNING("%s: on_group_check reply %s", name(), response.err.to_string());
+ LOG_WARNING_PREFIX("on_group_check reply {}", response.err);
Review Comment:
```suggestion
LOG_WARNING_F("on_group_check reply {}", response.err);
```
##########
src/replica/replica_check.cpp:
##########
@@ -160,12 +158,12 @@ void replica::on_group_check(const group_check_request &request,
if (request.config.ballot < get_ballot()) {
response.err = ERR_VERSION_OUTDATED;
- LOG_WARNING("%s: on_group_check reply %s", name(), response.err.to_string());
+ LOG_WARNING_PREFIX("on_group_check reply {}", response.err);
return;
} else if (request.config.ballot > get_ballot()) {
if (!update_local_configuration(request.config)) {
response.err = ERR_INVALID_STATE;
- LOG_WARNING("%s: on_group_check reply %s", name(), response.err.to_string());
+ LOG_WARNING_PREFIX("on_group_check reply {}", response.err);
Review Comment:
```suggestion
LOG_WARNING_F("on_group_check reply {}", response.err);
```
--
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 #1315: refactor(log): use LOG_WARNING_F instead of LOG_WARNING (1/3)
Posted by GitBox <gi...@apache.org>.
empiredan commented on code in PR #1315:
URL: https://github.com/apache/incubator-pegasus/pull/1315#discussion_r1073106992
##########
src/replica/replica_learn.cpp:
##########
@@ -178,15 +173,15 @@ void replica::init_learn(uint64_t signature)
if (_app->last_committed_decree() == 0 &&
_stub->_learn_app_concurrent_count.load() >= _options->learn_app_max_concurrent_count) {
- LOG_WARNING("%s: init_learn[%016" PRIx64 "]: learnee = %s, learn_duration = %" PRIu64
- "ms, need to learn app because app_committed_decree = 0, but "
- "learn_app_concurrent_count(%d) >= learn_app_max_concurrent_count(%d), skip",
- name(),
- _potential_secondary_states.learning_version,
- _config.primary.to_string(),
- _potential_secondary_states.duration_ms(),
- _stub->_learn_app_concurrent_count.load(),
- _options->learn_app_max_concurrent_count);
+ LOG_WARNING_PREFIX(
+ "init_learn[{:#018x}]: learnee = {}, learn_duration = {} ms, need to learn app "
+ "because app_committed_decree = 0, but learn_app_concurrent_count({}) >= "
+ "learn_app_max_concurrent_count({}), skip",
+ _potential_secondary_states.learning_version,
+ _config.primary,
+ _potential_secondary_states.duration_ms(),
+ _stub->_learn_app_concurrent_count.load(),
Review Comment:
```suggestion
_stub->_learn_app_concurrent_count,
```
##########
src/replica/replica_learn.cpp:
##########
@@ -694,14 +685,13 @@ void replica::on_learn_reply(error_code err, learn_request &&req, learn_response
if (resp.type == learn_type::LT_APP) {
if (++_stub->_learn_app_concurrent_count > _options->learn_app_max_concurrent_count) {
--_stub->_learn_app_concurrent_count;
- LOG_WARNING("%s: on_learn_reply[%016" PRIx64
- "]: learnee = %s, learn_app_concurrent_count(%d) >= "
- "learn_app_max_concurrent_count(%d), skip this round",
- name(),
- _potential_secondary_states.learning_version,
- _config.primary.to_string(),
- _stub->_learn_app_concurrent_count.load(),
- _options->learn_app_max_concurrent_count);
+ LOG_WARNING_PREFIX(
+ "on_learn_reply[{:#018x}]: learnee = {}, learn_app_concurrent_count({}) >= "
+ "learn_app_max_concurrent_count({}), skip this round",
+ _potential_secondary_states.learning_version,
+ _config.primary,
+ _stub->_learn_app_concurrent_count.load(),
Review Comment:
```suggestion
_stub->_learn_app_concurrent_count,
```
--
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 #1315: refactor(log): use LOG_WARNING_F instead of LOG_WARNING (1/3)
Posted by GitBox <gi...@apache.org>.
empiredan merged PR #1315:
URL: https://github.com/apache/incubator-pegasus/pull/1315
--
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] neverchanje commented on a diff in pull request #1315: refactor(log): use LOG_WARNING_F instead of LOG_WARNING (1/3)
Posted by GitBox <gi...@apache.org>.
neverchanje commented on code in PR #1315:
URL: https://github.com/apache/incubator-pegasus/pull/1315#discussion_r1073088455
##########
src/replica/replica_check.cpp:
##########
@@ -160,12 +158,12 @@ void replica::on_group_check(const group_check_request &request,
if (request.config.ballot < get_ballot()) {
response.err = ERR_VERSION_OUTDATED;
- LOG_WARNING("%s: on_group_check reply %s", name(), response.err.to_string());
+ LOG_WARNING_PREFIX("on_group_check reply {}", response.err);
Review Comment:
I see. Thanks. IIRC it was called `warnf_replica` or something similar. LOG_WARNING_PREFIX makes me thinking that the first argument is the 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] acelyc111 commented on a diff in pull request #1315: refactor(log): use LOG_WARNING_F instead of LOG_WARNING (1/3)
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on code in PR #1315:
URL: https://github.com/apache/incubator-pegasus/pull/1315#discussion_r1072407723
##########
src/replica/mutation_log.cpp:
##########
@@ -997,26 +996,26 @@ int64_t mutation_log::on_partition_reset(gpid gpid, decree max_decree)
replica_log_info old_info = _private_log_info;
_private_log_info.max_decree = max_decree;
_private_log_info.valid_start_offset = _global_end_offset;
- LOG_WARNING("replica %d.%d has changed private log max_decree from %" PRId64 " to %" PRId64
- ", valid_start_offset from %" PRId64 " to %" PRId64,
- gpid.get_app_id(),
- gpid.get_partition_index(),
- old_info.max_decree,
- _private_log_info.max_decree,
- old_info.valid_start_offset,
- _private_log_info.valid_start_offset);
+ LOG_WARNING_F("replica {}.{} has changed private log max_decree from {} to {}, "
+ "valid_start_offset from {} to {}",
+ gpid,
+ gpid.get_partition_index(),
+ old_info.max_decree,
+ _private_log_info.max_decree,
+ old_info.valid_start_offset,
+ _private_log_info.valid_start_offset);
Review Comment:
```suggestion
LOG_WARNING_F("replica {} has changed private log max_decree from {} to {}, "
"valid_start_offset from {} to {}",
gpid,
old_info.max_decree,
_private_log_info.max_decree,
old_info.valid_start_offset,
_private_log_info.valid_start_offset);
```
##########
src/replica/mutation_log.cpp:
##########
@@ -997,26 +996,26 @@ int64_t mutation_log::on_partition_reset(gpid gpid, decree max_decree)
replica_log_info old_info = _private_log_info;
_private_log_info.max_decree = max_decree;
_private_log_info.valid_start_offset = _global_end_offset;
- LOG_WARNING("replica %d.%d has changed private log max_decree from %" PRId64 " to %" PRId64
- ", valid_start_offset from %" PRId64 " to %" PRId64,
- gpid.get_app_id(),
- gpid.get_partition_index(),
- old_info.max_decree,
- _private_log_info.max_decree,
- old_info.valid_start_offset,
- _private_log_info.valid_start_offset);
+ LOG_WARNING_F("replica {}.{} has changed private log max_decree from {} to {}, "
+ "valid_start_offset from {} to {}",
+ gpid,
+ gpid.get_partition_index(),
+ old_info.max_decree,
+ _private_log_info.max_decree,
+ old_info.valid_start_offset,
+ _private_log_info.valid_start_offset);
} else {
replica_log_info info(max_decree, _global_end_offset);
auto it = _shared_log_info_map.insert(replica_log_info_map::value_type(gpid, info));
if (!it.second) {
- LOG_WARNING("replica %d.%d has changed shared log max_decree from %" PRId64
- " to %" PRId64 ", valid_start_offset from %" PRId64 " to %" PRId64,
- gpid.get_app_id(),
- gpid.get_partition_index(),
- it.first->second.max_decree,
- info.max_decree,
- it.first->second.valid_start_offset,
- info.valid_start_offset);
+ LOG_WARNING_F("replica {}.{} has changed shared log max_decree from {} to {}, "
+ "valid_start_offset from {} to {} ",
+ gpid,
+ gpid.get_partition_index(),
+ it.first->second.max_decree,
+ info.max_decree,
+ it.first->second.valid_start_offset,
+ info.valid_start_offset);
Review Comment:
```suggestion
LOG_WARNING_F("replica {} has changed shared log max_decree from {} to {}, "
"valid_start_offset from {} to {} ",
gpid,
it.first->second.max_decree,
info.max_decree,
it.first->second.valid_start_offset,
info.valid_start_offset);
```
--
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 #1315: refactor(log): use LOG_WARNING_F instead of LOG_WARNING (1/3)
Posted by GitBox <gi...@apache.org>.
WHBANG commented on code in PR #1315:
URL: https://github.com/apache/incubator-pegasus/pull/1315#discussion_r1073025939
##########
src/replica/replica_check.cpp:
##########
@@ -160,12 +158,12 @@ void replica::on_group_check(const group_check_request &request,
if (request.config.ballot < get_ballot()) {
response.err = ERR_VERSION_OUTDATED;
- LOG_WARNING("%s: on_group_check reply %s", name(), response.err.to_string());
+ LOG_WARNING_PREFIX("on_group_check reply {}", response.err);
Review Comment:
Thank you for your valuable comments
_ PREFIX can print _name of the class. class replica_check inherits class replica, class replica inherits replica_base, and replica_base implements the method of print _name, so there is no problem
--
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] neverchanje commented on a diff in pull request #1315: refactor(log): use LOG_WARNING_F instead of LOG_WARNING (1/3)
Posted by GitBox <gi...@apache.org>.
neverchanje commented on code in PR #1315:
URL: https://github.com/apache/incubator-pegasus/pull/1315#discussion_r1073088455
##########
src/replica/replica_check.cpp:
##########
@@ -160,12 +158,12 @@ void replica::on_group_check(const group_check_request &request,
if (request.config.ballot < get_ballot()) {
response.err = ERR_VERSION_OUTDATED;
- LOG_WARNING("%s: on_group_check reply %s", name(), response.err.to_string());
+ LOG_WARNING_PREFIX("on_group_check reply {}", response.err);
Review Comment:
I see. Thanks. IIRC it was called `warn_replica` or something similar. LOG_WARNING_PREFIX makes me thinking that the first argument is the 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] neverchanje commented on a diff in pull request #1315: refactor(log): use LOG_WARNING_F instead of LOG_WARNING (1/3)
Posted by GitBox <gi...@apache.org>.
neverchanje commented on code in PR #1315:
URL: https://github.com/apache/incubator-pegasus/pull/1315#discussion_r1073088455
##########
src/replica/replica_check.cpp:
##########
@@ -160,12 +158,12 @@ void replica::on_group_check(const group_check_request &request,
if (request.config.ballot < get_ballot()) {
response.err = ERR_VERSION_OUTDATED;
- LOG_WARNING("%s: on_group_check reply %s", name(), response.err.to_string());
+ LOG_WARNING_PREFIX("on_group_check reply {}", response.err);
Review Comment:
I see. Thanks. IIRC it as called `LOG_REPLICA` or something similar. LOG_WARNING_PREFIX makes me thinking that the first argument is the prefix.
##########
src/replica/replica_check.cpp:
##########
@@ -160,12 +158,12 @@ void replica::on_group_check(const group_check_request &request,
if (request.config.ballot < get_ballot()) {
response.err = ERR_VERSION_OUTDATED;
- LOG_WARNING("%s: on_group_check reply %s", name(), response.err.to_string());
+ LOG_WARNING_PREFIX("on_group_check reply {}", response.err);
Review Comment:
I see. Thanks. IIRC it was called `LOG_REPLICA` or something similar. LOG_WARNING_PREFIX makes me thinking that the first argument is the 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 #1315: refactor(log): use LOG_WARNING_F instead of LOG_WARNING (1/3)
Posted by GitBox <gi...@apache.org>.
WHBANG commented on code in PR #1315:
URL: https://github.com/apache/incubator-pegasus/pull/1315#discussion_r1072468592
##########
src/replica/mutation_log.cpp:
##########
@@ -997,26 +996,26 @@ int64_t mutation_log::on_partition_reset(gpid gpid, decree max_decree)
replica_log_info old_info = _private_log_info;
_private_log_info.max_decree = max_decree;
_private_log_info.valid_start_offset = _global_end_offset;
- LOG_WARNING("replica %d.%d has changed private log max_decree from %" PRId64 " to %" PRId64
- ", valid_start_offset from %" PRId64 " to %" PRId64,
- gpid.get_app_id(),
- gpid.get_partition_index(),
- old_info.max_decree,
- _private_log_info.max_decree,
- old_info.valid_start_offset,
- _private_log_info.valid_start_offset);
+ LOG_WARNING_F("replica {}.{} has changed private log max_decree from {} to {}, "
+ "valid_start_offset from {} to {}",
+ gpid,
+ gpid.get_partition_index(),
+ old_info.max_decree,
+ _private_log_info.max_decree,
+ old_info.valid_start_offset,
+ _private_log_info.valid_start_offset);
Review Comment:
done
##########
src/replica/mutation_log.cpp:
##########
@@ -997,26 +996,26 @@ int64_t mutation_log::on_partition_reset(gpid gpid, decree max_decree)
replica_log_info old_info = _private_log_info;
_private_log_info.max_decree = max_decree;
_private_log_info.valid_start_offset = _global_end_offset;
- LOG_WARNING("replica %d.%d has changed private log max_decree from %" PRId64 " to %" PRId64
- ", valid_start_offset from %" PRId64 " to %" PRId64,
- gpid.get_app_id(),
- gpid.get_partition_index(),
- old_info.max_decree,
- _private_log_info.max_decree,
- old_info.valid_start_offset,
- _private_log_info.valid_start_offset);
+ LOG_WARNING_F("replica {}.{} has changed private log max_decree from {} to {}, "
+ "valid_start_offset from {} to {}",
+ gpid,
+ gpid.get_partition_index(),
+ old_info.max_decree,
+ _private_log_info.max_decree,
+ old_info.valid_start_offset,
+ _private_log_info.valid_start_offset);
} else {
replica_log_info info(max_decree, _global_end_offset);
auto it = _shared_log_info_map.insert(replica_log_info_map::value_type(gpid, info));
if (!it.second) {
- LOG_WARNING("replica %d.%d has changed shared log max_decree from %" PRId64
- " to %" PRId64 ", valid_start_offset from %" PRId64 " to %" PRId64,
- gpid.get_app_id(),
- gpid.get_partition_index(),
- it.first->second.max_decree,
- info.max_decree,
- it.first->second.valid_start_offset,
- info.valid_start_offset);
+ LOG_WARNING_F("replica {}.{} has changed shared log max_decree from {} to {}, "
+ "valid_start_offset from {} to {} ",
+ gpid,
+ gpid.get_partition_index(),
+ it.first->second.max_decree,
+ info.max_decree,
+ it.first->second.valid_start_offset,
+ info.valid_start_offset);
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