acelyc111 commented on code in PR #1996:
URL:
https://github.com/apache/incubator-pegasus/pull/1996#discussion_r1595366263
##########
src/replica/replica_chkpt.cpp:
##########
@@ -112,66 +112,71 @@ void replica::on_checkpoint_timer()
init_checkpoint(false);
}
- if (_private_log) {
- mutation_log_ptr plog = _private_log;
-
- decree last_durable_decree = _app->last_durable_decree();
- decree min_confirmed_decree = _duplication_mgr->min_confirmed_decree();
- decree cleanable_decree = last_durable_decree;
- int64_t valid_start_offset =
_app->init_info().init_offset_in_private_log;
-
- if (min_confirmed_decree >= 0) {
- // Do not rely on valid_start_offset for GC during duplication.
- // cleanable_decree is the only GC trigger.
- valid_start_offset = 0;
- if (min_confirmed_decree < last_durable_decree) {
- LOG_INFO_PREFIX("gc_private {}: delay gc for duplication:
min_confirmed_decree({}) "
- "last_durable_decree({})",
- enum_to_string(status()),
- min_confirmed_decree,
- last_durable_decree);
- cleanable_decree = min_confirmed_decree;
- } else {
- LOG_INFO_PREFIX("gc_private {}: min_confirmed_decree({})
last_durable_decree({})",
- enum_to_string(status()),
- min_confirmed_decree,
- last_durable_decree);
- }
- } else if (is_duplication_master()) {
- // unsure if the logs can be dropped, because min_confirmed_decree
- // is currently unavailable
- LOG_INFO_PREFIX(
- "gc_private {}: skip gc because confirmed duplication progress
is unknown",
- enum_to_string(status()));
- return;
- }
+ if (_private_log == nullptr) {
+ return;
+ }
- if (is_duplication_plog_checking()) {
- LOG_DEBUG_PREFIX("gc_private {}: skip gc because duplication is
checking plog files",
- enum_to_string(status()));
- return;
- }
+ if (!is_plog_gc_enabled()) {
+ LOG_INFO_PREFIX("gc_private {}: skip gc because plog gc is disabled",
+ enum_to_string(status()));
+ return;
+ }
- tasking::enqueue(LPC_GARBAGE_COLLECT_LOGS_AND_REPLICAS,
- &_tracker,
- [this, plog, cleanable_decree, valid_start_offset] {
- // run in background thread to avoid file
deletion operation blocking
- // replication thread.
- if (status() == partition_status::PS_ERROR ||
- status() == partition_status::PS_INACTIVE)
- return;
- plog->garbage_collection(
- get_gpid(),
- cleanable_decree,
- valid_start_offset,
-
(int64_t)FLAGS_log_private_reserve_max_size_mb * 1024 * 1024,
-
(int64_t)FLAGS_log_private_reserve_max_time_seconds);
- if (status() == partition_status::PS_PRIMARY) {
- METRIC_VAR_SET(private_log_size_mb,
- _private_log->total_size() >>
20);
- }
- });
+ if (is_duplication_plog_checking()) {
+ LOG_INFO_PREFIX("gc_private {}: skip gc because duplication is
checking plog files",
+ enum_to_string(status()));
+ return;
}
+
+ mutation_log_ptr plog = _private_log;
+
+ decree last_durable_decree = _app->last_durable_decree();
+ decree min_confirmed_decree = _duplication_mgr->min_confirmed_decree();
+ decree cleanable_decree = last_durable_decree;
+ int64_t valid_start_offset = _app->init_info().init_offset_in_private_log;
+
+ if (min_confirmed_decree >= 0) {
+ // Do not rely on valid_start_offset for GC during duplication.
+ // cleanable_decree is the only GC trigger.
+ valid_start_offset = 0;
+ if (min_confirmed_decree < last_durable_decree) {
+ LOG_INFO_PREFIX("gc_private {}: delay gc for duplication:
min_confirmed_decree({}) "
+ "last_durable_decree({})",
+ enum_to_string(status()),
+ min_confirmed_decree,
+ last_durable_decree);
+ cleanable_decree = min_confirmed_decree;
+ } else {
+ LOG_INFO_PREFIX("gc_private {}: min_confirmed_decree({})
last_durable_decree({})",
+ enum_to_string(status()),
+ min_confirmed_decree,
+ last_durable_decree);
+ }
+ } else if (is_duplication_master()) {
Review Comment:
How about return earier like:
```
if (is_duplication_master() && min_confirmed_decree < 0) {
...
return;
}
if (min_confirmed_decree >= 0) {
...
}
```
##########
src/replica/replica_chkpt.cpp:
##########
@@ -112,66 +112,71 @@ void replica::on_checkpoint_timer()
init_checkpoint(false);
}
- if (_private_log) {
- mutation_log_ptr plog = _private_log;
-
- decree last_durable_decree = _app->last_durable_decree();
- decree min_confirmed_decree = _duplication_mgr->min_confirmed_decree();
- decree cleanable_decree = last_durable_decree;
- int64_t valid_start_offset =
_app->init_info().init_offset_in_private_log;
-
- if (min_confirmed_decree >= 0) {
- // Do not rely on valid_start_offset for GC during duplication.
- // cleanable_decree is the only GC trigger.
- valid_start_offset = 0;
- if (min_confirmed_decree < last_durable_decree) {
- LOG_INFO_PREFIX("gc_private {}: delay gc for duplication:
min_confirmed_decree({}) "
- "last_durable_decree({})",
- enum_to_string(status()),
- min_confirmed_decree,
- last_durable_decree);
- cleanable_decree = min_confirmed_decree;
- } else {
- LOG_INFO_PREFIX("gc_private {}: min_confirmed_decree({})
last_durable_decree({})",
- enum_to_string(status()),
- min_confirmed_decree,
- last_durable_decree);
- }
- } else if (is_duplication_master()) {
- // unsure if the logs can be dropped, because min_confirmed_decree
- // is currently unavailable
- LOG_INFO_PREFIX(
- "gc_private {}: skip gc because confirmed duplication progress
is unknown",
- enum_to_string(status()));
- return;
- }
+ if (_private_log == nullptr) {
+ return;
+ }
- if (is_duplication_plog_checking()) {
- LOG_DEBUG_PREFIX("gc_private {}: skip gc because duplication is
checking plog files",
- enum_to_string(status()));
- return;
- }
+ if (!is_plog_gc_enabled()) {
+ LOG_INFO_PREFIX("gc_private {}: skip gc because plog gc is disabled",
Review Comment:
How about using WARNING level logs?
##########
src/replica/replica_chkpt.cpp:
##########
@@ -112,66 +112,71 @@ void replica::on_checkpoint_timer()
init_checkpoint(false);
}
- if (_private_log) {
- mutation_log_ptr plog = _private_log;
-
- decree last_durable_decree = _app->last_durable_decree();
- decree min_confirmed_decree = _duplication_mgr->min_confirmed_decree();
- decree cleanable_decree = last_durable_decree;
- int64_t valid_start_offset =
_app->init_info().init_offset_in_private_log;
-
- if (min_confirmed_decree >= 0) {
- // Do not rely on valid_start_offset for GC during duplication.
- // cleanable_decree is the only GC trigger.
- valid_start_offset = 0;
- if (min_confirmed_decree < last_durable_decree) {
- LOG_INFO_PREFIX("gc_private {}: delay gc for duplication:
min_confirmed_decree({}) "
- "last_durable_decree({})",
- enum_to_string(status()),
- min_confirmed_decree,
- last_durable_decree);
- cleanable_decree = min_confirmed_decree;
- } else {
- LOG_INFO_PREFIX("gc_private {}: min_confirmed_decree({})
last_durable_decree({})",
- enum_to_string(status()),
- min_confirmed_decree,
- last_durable_decree);
- }
- } else if (is_duplication_master()) {
- // unsure if the logs can be dropped, because min_confirmed_decree
- // is currently unavailable
- LOG_INFO_PREFIX(
- "gc_private {}: skip gc because confirmed duplication progress
is unknown",
- enum_to_string(status()));
- return;
- }
+ if (_private_log == nullptr) {
+ return;
+ }
- if (is_duplication_plog_checking()) {
- LOG_DEBUG_PREFIX("gc_private {}: skip gc because duplication is
checking plog files",
- enum_to_string(status()));
- return;
- }
+ if (!is_plog_gc_enabled()) {
+ LOG_INFO_PREFIX("gc_private {}: skip gc because plog gc is disabled",
+ enum_to_string(status()));
+ return;
+ }
- tasking::enqueue(LPC_GARBAGE_COLLECT_LOGS_AND_REPLICAS,
- &_tracker,
- [this, plog, cleanable_decree, valid_start_offset] {
- // run in background thread to avoid file
deletion operation blocking
- // replication thread.
- if (status() == partition_status::PS_ERROR ||
- status() == partition_status::PS_INACTIVE)
- return;
- plog->garbage_collection(
- get_gpid(),
- cleanable_decree,
- valid_start_offset,
-
(int64_t)FLAGS_log_private_reserve_max_size_mb * 1024 * 1024,
-
(int64_t)FLAGS_log_private_reserve_max_time_seconds);
- if (status() == partition_status::PS_PRIMARY) {
- METRIC_VAR_SET(private_log_size_mb,
- _private_log->total_size() >>
20);
- }
- });
+ if (is_duplication_plog_checking()) {
+ LOG_INFO_PREFIX("gc_private {}: skip gc because duplication is
checking plog files",
+ enum_to_string(status()));
+ return;
}
+
+ mutation_log_ptr plog = _private_log;
+
+ decree last_durable_decree = _app->last_durable_decree();
+ decree min_confirmed_decree = _duplication_mgr->min_confirmed_decree();
+ decree cleanable_decree = last_durable_decree;
+ int64_t valid_start_offset = _app->init_info().init_offset_in_private_log;
+
+ if (min_confirmed_decree >= 0) {
+ // Do not rely on valid_start_offset for GC during duplication.
+ // cleanable_decree is the only GC trigger.
+ valid_start_offset = 0;
+ if (min_confirmed_decree < last_durable_decree) {
+ LOG_INFO_PREFIX("gc_private {}: delay gc for duplication:
min_confirmed_decree({}) "
+ "last_durable_decree({})",
+ enum_to_string(status()),
+ min_confirmed_decree,
+ last_durable_decree);
+ cleanable_decree = min_confirmed_decree;
+ } else {
+ LOG_INFO_PREFIX("gc_private {}: min_confirmed_decree({})
last_durable_decree({})",
+ enum_to_string(status()),
+ min_confirmed_decree,
+ last_durable_decree);
+ }
+ } else if (is_duplication_master()) {
+ // unsure if the logs can be dropped, because min_confirmed_decree
+ // is currently unavailable
+ LOG_INFO_PREFIX("gc_private {}: skip gc because confirmed duplication
progress is unknown",
+ enum_to_string(status()));
+ return;
+ }
+
+ tasking::enqueue(
+ LPC_GARBAGE_COLLECT_LOGS_AND_REPLICAS,
+ &_tracker,
+ [this, plog, cleanable_decree, valid_start_offset] {
+ // run in background thread to avoid file deletion operation
blocking
+ // replication thread.
+ if (status() == partition_status::PS_ERROR || status() ==
partition_status::PS_INACTIVE)
+ return;
Review Comment:
```suggestion
if (status() == partition_status::PS_ERROR || status() ==
partition_status::PS_INACTIVE) {
return;
}
```
##########
src/replica/replica_stub.cpp:
##########
@@ -2299,6 +2299,41 @@ void replica_stub::register_ctrl_command()
});
}));
+
_cmds.emplace_back(::dsn::command_manager::instance().register_single_command(
Review Comment:
How about simplify the commands in format of `replica.plog-gc [true|false]`
by `register_bool_command()`, like some other remote-commands, e.g.
`replica.deny-client`.
--
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]
