You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pegasus.apache.org by "acelyc111 (via GitHub)" <gi...@apache.org> on 2024/05/09 12:16:10 UTC
Re: [PR] feat(plog): garbage collection is allowed to be enabled/disabled for plog [incubator-pegasus]
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: 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