empiredan commented on code in PR #1667:
URL: 
https://github.com/apache/incubator-pegasus/pull/1667#discussion_r1382774626


##########
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:
   OK, I'll add test cases for both `is_data_dir_removable(bool)` and 
`is_data_dir_invalid(bool)`.



-- 
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]

Reply via email to