acelyc111 commented on code in PR #1667:
URL:
https://github.com/apache/incubator-pegasus/pull/1667#discussion_r1381156821
##########
src/replica/disk_cleaner.cpp:
##########
@@ -87,59 +177,85 @@ error_s disk_remove_useless_dirs(const
std::vector<std::shared_ptr<dir_node>> &d
}
sub_list.insert(sub_list.end(), tmp_list.begin(), tmp_list.end());
}
- for (auto &fpath : sub_list) {
- auto name = dsn::utils::filesystem::get_file_name(fpath);
- if (!is_data_dir_removable(name)) {
- continue;
- }
- std::string folder_suffix = name.substr(name.length() - 4);
- time_t mt;
- if (!dsn::utils::filesystem::last_write_time(fpath, mt)) {
- LOG_WARNING("gc_disk: failed to get last write time of {}", fpath);
- continue;
- }
-
- auto last_write_time = (uint64_t)mt;
- uint64_t current_time_ms = dsn_now_ms();
- uint64_t remove_interval_seconds = current_time_ms / 1000;
+ for (const auto &path : sub_list) {
+ uint64_t expiration_seconds = 0;
- // don't delete ".bak" directory because it is backed by administrator.
- if (folder_suffix == kFolderSuffixErr) {
+ // Note: don't delete ".bak" directory since it could be did by
administrator.
+ const auto name = dsn::utils::filesystem::get_file_name(path);
+ if (boost::algorithm::ends_with(name, kFolderSuffixErr)) {
report.error_replica_count++;
- remove_interval_seconds =
FLAGS_gc_disk_error_replica_interval_seconds;
- } else if (folder_suffix == kFolderSuffixGar) {
+ if (!get_expiration_seconds_by_timestamp(name,
+ path,
+ kFolderSuffixErr.size(),
+
FLAGS_gc_disk_error_replica_interval_seconds,
+ expiration_seconds)) {
+ continue;
+ }
+ } else if (boost::algorithm::ends_with(name, kFolderSuffixGar)) {
report.garbage_replica_count++;
- remove_interval_seconds =
FLAGS_gc_disk_garbage_replica_interval_seconds;
- } else if (folder_suffix == kFolderSuffixTmp) {
+ if (!get_expiration_seconds_by_timestamp(name,
+ path,
+ kFolderSuffixGar.size(),
+
FLAGS_gc_disk_garbage_replica_interval_seconds,
+ expiration_seconds)) {
+ continue;
+ }
+ } else if (boost::algorithm::ends_with(name, kFolderSuffixTmp)) {
report.disk_migrate_tmp_count++;
- remove_interval_seconds =
FLAGS_gc_disk_migration_tmp_replica_interval_seconds;
- } else if (folder_suffix == kFolderSuffixOri) {
+ if (!get_expiration_seconds_by_last_write_time(
+ path,
+ FLAGS_gc_disk_migration_tmp_replica_interval_seconds,
+ expiration_seconds)) {
+ continue;
+ }
+ } else if (boost::algorithm::ends_with(name, kFolderSuffixOri)) {
report.disk_migrate_origin_count++;
- remove_interval_seconds =
FLAGS_gc_disk_migration_origin_replica_interval_seconds;
+ if (!get_expiration_seconds_by_last_write_time(
+ path,
+ FLAGS_gc_disk_migration_origin_replica_interval_seconds,
+ expiration_seconds)) {
+ continue;
+ }
+ } else {
+ continue;
}
- if (last_write_time + remove_interval_seconds <= current_time_ms /
1000) {
- if (!dsn::utils::filesystem::remove_path(fpath)) {
- LOG_WARNING("gc_disk: failed to delete directory '{}',
time_used_ms = {}",
- fpath,
- dsn_now_ms() - current_time_ms);
- } else {
+ const auto current_time_ms = dsn_now_ms();
+ if (expiration_seconds <= current_time_ms / 1000) {
+ if (dsn::utils::filesystem::remove_path(path)) {
LOG_WARNING("gc_disk: replica_dir_op succeed to delete
directory '{}'"
", time_used_ms = {}",
- fpath,
+ path,
dsn_now_ms() - current_time_ms);
report.remove_dir_count++;
+ } else {
+ LOG_WARNING("gc_disk: failed to delete directory '{}',
time_used_ms = {}",
+ path,
+ dsn_now_ms() - current_time_ms);
}
} else {
LOG_INFO("gc_disk: reserve directory '{}', wait_seconds = {}",
- fpath,
- last_write_time + remove_interval_seconds -
current_time_ms / 1000);
+ path,
+ expiration_seconds - current_time_ms / 1000);
}
}
return error_s::ok();
}
+bool is_data_dir_removable(const std::string &dir)
Review Comment:
Could you please add some unit tests for the 2 functions?
##########
src/replica/disk_cleaner.cpp:
##########
@@ -70,6 +75,91 @@ const std::string kFolderSuffixBak = ".bak";
const std::string kFolderSuffixOri = ".ori";
const std::string kFolderSuffixTmp = ".tmp";
+namespace {
+
+bool get_expiration_seconds_by_last_write_time(const std::string &path,
+ uint64_t delay_seconds,
+ uint64_t &expiration_seconds)
+{
+ time_t last_write_seconds;
+ if (!dsn::utils::filesystem::last_write_time(path, last_write_seconds)) {
+ LOG_WARNING("gc_disk: failed to get last write time of {}", path);
+ return false;
+ }
+
+ expiration_seconds = static_cast<uint64_t>(last_write_seconds) +
delay_seconds;
+ return true;
+}
+
+// Unix timestamp in microseconds for 2010-01-01 00:00:00.
+// This timestamp could be used as the minimum, since it's far earlier than
the time when
+// Pegasus was born.
+#define MIN_TIMESTAMP_US 1262275200000000
+#define MIN_TIMESTAMP_US_LENGTH (sizeof(STRINGIFY(MIN_TIMESTAMP_US)) - 1)
+
+bool parse_timestamp_us(const std::string &name, size_t suffix_size, uint64_t
×tamp_us)
+{
+ // Examples of the directory names of faulty or dropped replicas could be:
+ // 1.1.pegasus.1698843209235962.err
+ // 2.1.pegasus.1698843214240709.gar
Review Comment:
It would better to describe the parameters above the line of function
declaration.
Besides, give describe what the `suffix` is (to understand what
`suffix_size` would be), and example of `timestamp_us`
##########
src/replica/disk_cleaner.cpp:
##########
@@ -70,6 +75,91 @@ const std::string kFolderSuffixBak = ".bak";
const std::string kFolderSuffixOri = ".ori";
const std::string kFolderSuffixTmp = ".tmp";
+namespace {
+
+bool get_expiration_seconds_by_last_write_time(const std::string &path,
Review Comment:
Instead of `seconds`, maybe `timestamp` would be more meaningful.
--
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]
