From c418ad48bc85f2c8e3e6654b2aa2e46710315d0a Mon Sep 17 00:00:00 2001 From: Ning Sun Date: Tue, 21 Jul 2026 20:23:26 +0800 Subject: [PATCH] fix: address review comments --- src/mito2/src/engine/region_hook.rs | 41 ++++++++++++++++------------- src/mito2/src/gc.rs | 12 ++------- src/mito2/src/gc/worker_test.rs | 10 ------- 3 files changed, 24 insertions(+), 39 deletions(-) diff --git a/src/mito2/src/engine/region_hook.rs b/src/mito2/src/engine/region_hook.rs index d3eff73c695..4d816463b77 100644 --- a/src/mito2/src/engine/region_hook.rs +++ b/src/mito2/src/engine/region_hook.rs @@ -391,21 +391,28 @@ pub trait RegionHook: Send + Sync + Debug { /// Called after the datanode's global GC worker (`LocalGcWorker`) finishes a /// GC pass for a region — live regions (periodic GC) or dropped/repartitioned /// regions (the global reclamation path). Lets extensions with sidecar files - /// outside mito2's region dir clean up their own residual files. + /// outside mito2's region dir clean up residual files. /// - /// Authorization is scoped to stay safe against cross-region / tmp refs mito - /// honors but an extension can't see: - /// - [`RegionGcInfo::terminal`] `false`: clean only sidecar artifacts for the - /// files in [`RegionGcInfo::removed_files`] (plus orphans on a full-listing - /// pass). [`RegionGcInfo::is_region_dropped`] is context, not authorization. - /// - [`RegionGcInfo::terminal`] `true`: all refs released; may remove the - /// whole sidecar dir. + /// Always scoped to [`RegionGcInfo::removed_files`]: clean only sidecar + /// artifacts for the files mito deleted this pass. On a + /// [`RegionGcInfo::full_file_listing`] pass you may also reconcile + /// sidecar-only orphans (e.g. detect a fully-reaped region by cross-checking + /// the region dir against your own manifest). This callback never authorizes + /// blind whole-directory removal — derive "fully reaped" from your own state + /// so a stale mito snapshot can't cause accidental deletion. + /// [`RegionGcInfo::is_region_dropped`] is context, not authorization. /// - /// Returns `Err` to keep the region un-acknowledged (`need_retry_regions`) - /// so the next pass replays this callback. + /// # Retry + /// + /// Returning `Err` keeps the region un-acknowledged (`need_retry_regions`) + /// for a future replay. **Dropped/repartitioned**: guaranteed — metasrv keeps + /// the `table_repart` tombstone until `Ok`, so full-listing passes continue + /// until cleanup finishes. **Live**: best-effort — not expedited through + /// candidate selection, and a later full-listing pass may not reconstruct the + /// same `removed_files`; treat live cleanup as opportunistic. /// /// `region_metadata` is `None` for dropped regions; use `region_id` + - /// `access_layer`. Runs on the background GC task; must be idempotent. + /// `access_layer`. Idempotent; runs on the background GC task. async fn on_region_gc( &self, region_id: RegionId, @@ -421,19 +428,15 @@ pub trait RegionHook: Send + Sync + Debug { /// What mito2's GC pass deleted for a region, handed to /// [`RegionHook::on_region_gc`]. pub struct RegionGcInfo<'a> { - /// Files mito2 physically deleted this pass. When [`Self::terminal`] is - /// `false`, cleanup must be scoped to these. + /// Files mito2 physically deleted this pass. Cleanup must be scoped to these + /// (plus sidecar-only orphans you can identify on a [`Self::full_file_listing`] + /// pass). pub removed_files: &'a [RemovedFile], /// `true` when the region is dropped/absent (e.g. a repartitioned source). - /// Context only — not authorization to remove the whole sidecar dir; see - /// [`Self::terminal`]. `region_metadata` is `None` when this is `true`. + /// Context only — not authorization. `region_metadata` is `None` when `true`. pub is_region_dropped: bool, /// Whether this pass did a full object-store listing. pub full_file_listing: bool, - /// All refs to the region's files are released (dropped, no cross-region/tmp - /// refs). Only when `true` may an extension remove its whole sidecar dir; - /// otherwise stay scoped to [`Self::removed_files`]. - pub terminal: bool, } pub type RegionHookRef = Arc; diff --git a/src/mito2/src/gc.rs b/src/mito2/src/gc.rs index 4f59848b375..8c0e72fc083 100644 --- a/src/mito2/src/gc.rs +++ b/src/mito2/src/gc.rs @@ -567,15 +567,8 @@ impl LocalGcWorker { // Notify extensions so they can clean up sidecar files. Fire when there // are removed files, or on a full-listing pass (lets extensions reconcile - // orphans even when mito deleted nothing). `terminal` authorizes whole- - // sidecar-dir removal only for a dropped region with no remaining refs. - let terminal = is_region_dropped - && !self - .file_ref_manifest - .cross_region_refs - .contains_key(®ion_id) - && !self.file_ref_manifest.file_refs.contains_key(®ion_id); - + // orphans even when mito deleted nothing). Cleanup is always scoped to + // `removed_files`; see `RegionGcInfo`. let extension_cleanup_failed = if let Some(hook) = &self.hook && (!deletable_files.is_empty() || self.full_file_listing) { @@ -589,7 +582,6 @@ impl LocalGcWorker { removed_files: &deletable_files, is_region_dropped, full_file_listing: self.full_file_listing, - terminal, }, ) .await; diff --git a/src/mito2/src/gc/worker_test.rs b/src/mito2/src/gc/worker_test.rs index 939d63b5f72..1efa70e6e91 100644 --- a/src/mito2/src/gc/worker_test.rs +++ b/src/mito2/src/gc/worker_test.rs @@ -755,7 +755,6 @@ async fn test_file_in_tmp_ref_old_mtime_kept() { struct GcCountingHook { gc_calls: AtomicUsize, last_is_region_dropped: AtomicBool, - last_terminal: AtomicBool, fail_remaining: AtomicUsize, } @@ -771,7 +770,6 @@ impl RegionHook for GcCountingHook { self.gc_calls.fetch_add(1, Ordering::Relaxed); self.last_is_region_dropped .store(info.is_region_dropped, Ordering::Relaxed); - self.last_terminal.store(info.terminal, Ordering::Relaxed); // Atomically decrement-and-return the previous value; fail while > 0. let prev = self.fail_remaining.fetch_sub(1, Ordering::Relaxed); if prev > 0 { @@ -1048,10 +1046,6 @@ async fn test_on_region_gc_fires_for_dropped_region() { hook.last_is_region_dropped.load(Ordering::Relaxed), "a dropped region's GC pass must report is_region_dropped=true" ); - assert!( - hook.last_terminal.load(Ordering::Relaxed), - "a dropped region with no cross-region/tmp refs is terminally cleaned (terminal=true)" - ); } /// `on_region_gc` **fires** on a full-listing pass even when mito itself deleted @@ -1131,10 +1125,6 @@ async fn test_on_region_gc_fires_on_full_listing_without_deleted_files() { !hook.last_is_region_dropped.load(Ordering::Relaxed), "a live region's GC pass must report is_region_dropped=false" ); - assert!( - !hook.last_terminal.load(Ordering::Relaxed), - "a live region is never terminal" - ); } /// A failing `on_region_gc` must keep the region un-acknowledged: it lands in