Improve state transitions in blob_file_garbage_collector (WIP)

src/limestone/blob_file_garbage_collector.cpp

@@ -146,7 +146,6 @@
             std::lock_guard<std::mutex> lock(mutex_);
             cleanup_cv_.notify_all();
         }
-         reset();
      });
  }
 
@@ -204,6 +203,7 @@
     shutdown_requested_.store(true, std::memory_order_release);
     wait_for_all_threads();
     shutdown_requested_.store(false, std::memory_order_release);
+    reset();
  }
 
 
@@ -217,7 +217,6 @@ void blob_file_garbage_collector::wait_for_all_threads() {
     if (cleanup_thread_.joinable()) {
         cleanup_thread_.join();
     }
-    reset();
 }
 
 void blob_file_garbage_collector::scan_snapshot(const boost::filesystem::path &snapshot_file, const boost::filesystem::path &compacted_file) {
@@ -293,15 +292,16 @@ void blob_file_garbage_collector::wait_for_scan_snapshot() {
 
 
  void blob_file_garbage_collector::reset() {
-     std::lock_guard<std::mutex> lock(mutex_);
+    state_machine_.reset();
      scanned_blobs_ = std::make_unique<blob_id_container>();
      gc_exempt_blob_ = std::make_unique<blob_id_container>();
      max_existing_blob_id_ = 0;
  }
 
  bool blob_file_garbage_collector::is_active() const {
      blob_file_gc_state current_state = state_machine_.get_state();
-     return current_state != blob_file_gc_state::not_started;
+     return current_state != blob_file_gc_state::not_started &&
+            current_state != blob_file_gc_state::completed;
  }
 
  } // namespace limestone::internal

src/limestone/blob_file_gc_state_machine.cpp

@@ -123,6 +123,12 @@ static const std::unordered_map<state_event_pair, blob_file_gc_state> state_tran
     {{blob_file_gc_state::shutdown, blob_file_gc_event::complete_snapshot_scan}, blob_file_gc_state::shutdown},
     {{blob_file_gc_state::shutdown, blob_file_gc_event::complete_cleanup}, blob_file_gc_state::shutdown},
     {{blob_file_gc_state::shutdown, blob_file_gc_event::shutdown}, blob_file_gc_state::shutdown},
+
+    // =========================
+    // Reset
+    // =========================
+    {{blob_file_gc_state::not_started, blob_file_gc_event::reset}, blob_file_gc_state::not_started},
+    {{blob_file_gc_state::completed, blob_file_gc_event::reset}, blob_file_gc_state::not_started},
     {{blob_file_gc_state::shutdown, blob_file_gc_event::reset}, blob_file_gc_state::not_started}};
 
 // ================= blob_file_gc_state_machine =================

src/limestone/datastore.cpp

@@ -670,12 +670,14 @@ void datastore::compact_with_online() {
 
     // check blob file garbage collection runnable
     bool blob_file_gc_runnable = false;
-    if (boundary_version_copy.get_major() > compaction_catalog_->get_max_epoch_id() && blob_file_garbage_collector_->is_active()) {
+    bool is_active = blob_file_garbage_collector_->is_active();
+    if (boundary_version_copy.get_major() > compaction_catalog_->get_max_epoch_id() && !is_active) {
         blob_file_gc_runnable = true;
+        blob_file_garbage_collector_->shutdown();
     }
     VLOG_LP(log_trace_fine) << "boundary_version_copy.get_major(): " << boundary_version_copy.get_major()
                             << ", compaction_catalog_->get_max_epoch_id(): " << compaction_catalog_->get_max_epoch_id()
-                            << ", blob_file_garbage_collector_->is_active(): " << blob_file_garbage_collector_->is_active()
+                            << ", blob_file_garbage_collector_->is_active(): " << is_active
                             << ", blob_file_gc_runnable: " << blob_file_gc_runnable;
 
     // rotate first
@@ -762,9 +764,9 @@ void datastore::compact_with_online() {
     // blob files garbage collection
     if (options.is_gc_enabled()) {
         LOG_LP(INFO) << "start blob files garbage collection";
-        blob_file_garbage_collector_->wait_for_all_threads();
         blob_file_garbage_collector_->scan_blob_files(next_blob_id_copy);
         log_entry_container log_entries = options.get_gc_snapshot().finalize_snapshot();
+        blob_file_garbage_collector_->start_add_gc_exempt_blob_ids();
         for (const auto& entry : log_entries) {
             for (const auto& blob_id : entry.get_blob_ids()) {
                 blob_file_garbage_collector_->add_gc_exempt_blob_id(blob_id);

test/limestone/blob/blob_file_gc_state_machine_test.cpp

@@ -88,27 +88,6 @@ class blob_file_gc_state_machine_test : public ::testing::Test {
     }
 
 
-    /**
-     * @brief Test: Reset should only be allowed from shutdown.
-     */
-    TEST_F(blob_file_gc_state_machine_test, reset_only_allowed_from_shutdown) {
-        for (int s = static_cast<int>(blob_file_gc_state::not_started);
-                s <= static_cast<int>(blob_file_gc_state::shutdown); ++s) {
-
-            blob_file_gc_state current_state = static_cast<blob_file_gc_state>(s);
-            state_machine_.force_set_state(current_state);
-
-            if (current_state == blob_file_gc_state::shutdown) {
-                EXPECT_NO_THROW(state_machine_.transition(blob_file_gc_event::reset))
-                    << "Reset should be allowed from shutdown";
-            } else {
-                EXPECT_THROW(state_machine_.transition(blob_file_gc_event::reset), std::logic_error)
-                    << "Reset should only be allowed from shutdown, but was allowed from " 
-                    << blob_file_gc_state_machine::to_string(current_state);
-            }
-        }
-    }
-
     /**
      * @brief Test: Shutdown should always be allowed.
      */