feat: gc worker metasrv scheduler #6985

discord9 · 2025-09-17T08:16:54Z

I hereby agree to the terms of the GreptimeDB CLA.

Refer to a related PR or issue link (optional)

What's changed and what's your intention?

gc worker metasrv trigger, which trigger gc worker on datanode depending on file_removal_rate and sst_num

TODO: ~~still need to change the way datanode update removed_files to reduce the number of delete operation it send to object store~~ done, but maybe also need to persistent actually delete files in RegionEdit
TODO:

more unit tests&integration tests

PR Checklist

Please convert it to a draft if some of the following conditions are not met.

I have written the necessary rustdoc comments.
I have added the necessary unit tests and integration tests.
This PR requires documentation updates.
API changes are backward compatible.
Schema or data changes are backward compatible.

MichaelScofield

Lack of integration test?

src/meta-srv/src/gc/mailbox.rs

src/meta-srv/src/gc/options.rs

src/mito2/src/gc.rs

Copilot

Pull Request Overview

This PR implements a comprehensive garbage collection (GC) system for managing file lifecycle in the mito2 storage engine. The changes introduce metasrv-coordinated GC scheduling, file removal rate tracking, and improved manifest management for tracking deleted files.

Key changes:

Added GC scheduler on metasrv to coordinate garbage collection across datanodes
Implemented file removal rate tracking to monitor GC pressure and prioritize regions
Refactored manifest management to use reference-based statistics and removed hardcoded file retention policies
Changed lingering_time from required to optional, allowing immediate file deletion when set to None

Reviewed Changes

Copilot reviewed 44 out of 45 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/store-api/src/region_engine.rs	Added `file_removal_rate` field to `RegionManifestInfo` and derived `Hash` for `RegionRole`
src/mito2/src/region.rs	Added `file_removal_rate` to `ManifestStats` and exposed it in region statistics
src/mito2/src/manifest/manager.rs	Refactored to use `ManifestStats` reference instead of individual atomic fields
src/mito2/src/manifest/action.rs	Added file removal rate calculation and removed TTL-based file eviction logic
src/mito2/src/gc.rs	Refactored GC worker to accept region references and made `lingering_time` optional
src/mito2/src/sst/file_purger.rs	Added `gc_enabled` parameter to determine purger type selection
src/meta-srv/src/gc/*.rs	New GC scheduling infrastructure including scheduler, tracker, candidate selection, and mailbox communication
src/datanode/src/heartbeat/handler/gc_worker.rs	Updated to handle GC instructions with improved validation
tests-integration/tests/http.rs	Reduced default lingering times for faster testing
src/mito2/src/gc/worker_test.rs	New integration tests for GC worker functionality

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-05T08:17:31Z

src/mito2/src/manifest/action.rs

+    fn file_removed_cnt_after(&self, t_ms: i64) -> (u64, Option<i64>) {
+        let mut cnt = 0;
+        let mut min_ts_after: Option<i64> = None;
+        for record in &self.removed_files {
+            if record.removed_at >= t_ms {
+                cnt += record.file_ids.len();
+            }
+            min_ts_after = match min_ts_after {
+                Some(ts) => Some(ts.min(record.removed_at)),
+                None => Some(record.removed_at),
+            };
+        }
+        (cnt as u64, min_ts_after)
+    }


The min_ts_after calculation is incorrect. It computes the minimum timestamp across ALL removed files, not just those after t_ms. This should only track the minimum timestamp of files matching the condition removed_at >= t_ms. The current logic will return timestamps from before the threshold, leading to incorrect file removal rate calculations.

Copilot · 2025-11-05T08:17:32Z

src/mito2/src/manifest/action.rs

+    /// Count the number of files removed after the given timestamp. Also return the minimum
+    /// timestamp of all removed files.


The documentation says 'minimum timestamp of all removed files' but based on the function name and first part of the comment, it should say 'minimum timestamp of removed files after the given timestamp' to match the intended behavior.

Copilot · 2025-11-05T08:17:32Z

src/mito2/src/gc.rs

+            // expect long running queries to be finished(or at least be able to notify it's using a deleted file) within a reasonable time
+            lingering_time: Some(Duration::from_secs(60)),


Corrected spelling of 'it's' to 'they're' for grammatical correctness. Should be 'they're using a deleted file' since 'queries' is plural.

killme2008 · 2025-11-05T08:27:06Z

src/meta-srv/src/gc/candidate.rs

+                file_removal_rate, ..
+            } => *file_removal_rate as f64 * self.config.file_removal_rate_weight,
+            // Metric engine doesn't have file_removal_rate, also this should be unreachable since metrics engine doesn't support gc
+            RegionManifestInfo::Metric { .. } => 0.0,


Better to panic?

killme2008 · 2025-11-05T08:27:28Z

src/meta-srv/src/gc/candidate.rs

+}
+
+impl GcScheduler {
+    /// Calculate GC priority score for a region based on various metrics.


It's better to doc the alogorithm.

killme2008 · 2025-11-05T08:29:26Z

src/meta-srv/src/gc/ctx.rs

+    /// The mailbox to send messages.
+    pub(crate) mailbox: MailboxRef,
+    /// The server address.
+    pub(crate) server_addr: String,


Who's server address?

killme2008 · 2025-11-05T08:30:36Z

src/meta-srv/src/gc/ctx.rs

+        let dn_stats = self.meta_peer_client.get_all_dn_stat_kvs().await?;
+        let mut table_to_region_stats: HashMap<TableId, Vec<RegionStat>> = HashMap::new();
+        for (_dn_id, stats) in dn_stats {
+            let mut stats = stats.stats;


Aren’t the stats already sorted by timestamp? I’m not sure, but it seems like they should be.

killme2008 · 2025-11-05T08:31:13Z

src/meta-srv/src/gc/ctx.rs

+use crate::service::mailbox::{Channel, MailboxRef};
+
+#[async_trait::async_trait]
+pub(crate) trait SchedulerCtx: Send + Sync {


nit: love to doc the trait and functions

killme2008 · 2025-11-05T08:46:57Z

src/meta-srv/src/gc/options.rs

+    fn default() -> Self {
+        Self {
+            enable: false,
+            max_concurrent_tables: 10,


Concurrency seems high; let's be conservative in the initial implementation.

killme2008 · 2025-11-05T08:48:08Z

src/meta-srv/src/gc/options.rs

+impl GcSchedulerOptions {
+    /// Validates the configuration options.
+    pub fn validate(&self) -> Result<()> {
+        if self.max_concurrent_tables == 0 {


Replacing all these validations with ensure! looks better.

killme2008 · 2025-11-05T08:48:58Z

src/meta-srv/src/gc/scheduler.rs

+        while let Some(event) = self.receiver.recv().await {
+            match event {
+                Event::Tick => {
+                    info!("Received gc tick");


Annoying log

Suggested change

info!("Received gc tick");

killme2008 · 2025-11-05T08:58:07Z

src/mito2/src/gc.rs

-            // 6 hours, for unknown expel time, which is when this file get removed from manifest, it should rarely happen, can keep it longer
-            unknown_file_lingering_time: Duration::from_secs(60 * 60 * 6),
+            // expect long running queries to be finished(or at least be able to notify it's using a deleted file) within a reasonable time
+            lingering_time: Some(Duration::from_secs(60)),


Why change lingering time smaller?

killme2008 · 2025-11-05T08:59:07Z

src/mito2/src/gc.rs

            "Successfully deleted {} unused files for region {}",
            unused_len, region_id
        );
+        // TODO(discord9): update region manifest about deleted files


The todo is outdated.

killme2008 · 2025-11-05T09:05:19Z

I’m wondering whether running the GC worker concurrently with migration or repartition tasks could lead to any concurrency issues. This is something I’m particularly concerned about. @waynexia @WenyXu @fengjiachun

I’ve noticed that the current logic relies on certain assumptions — for example, that a region resides on the local datanode. However, these assumptions might not always hold true. It’s possible that the conditions are valid at the time of verification but become invalid during execution, leading to constraint violations.

The core question is: if these assumptions or checks are invalidated during execution due to ongoing migration or repartition operations, what would be the resulting behavior?

If such consistency cannot be guaranteed, then it would be safer to execute these processes sequentially rather than in parallel.

Signed-off-by: discord9 <[email protected]>

Signed-off-by: discord9 <[email protected]> feat: gc scheduler wip: gc trigger Signed-off-by: discord9 <[email protected]> feat: dn file removal rate Signed-off-by: discord9 <[email protected]> feat: trigger gc with stats(WIP) Signed-off-by: discord9 <[email protected]> chore Signed-off-by: discord9 <[email protected]> also move files ref manifest to store-api Signed-off-by: discord9 <[email protected]> feat: basic gc trigger impl Signed-off-by: discord9 <[email protected]> wip: handle file ref change Signed-off-by: discord9 <[email protected]> refactor: use region ids Signed-off-by: discord9 <[email protected]> fix: retry using related regions Signed-off-by: discord9 <[email protected]> chore: rm unused Signed-off-by: discord9 <[email protected]> fix: update file reference type in GC worker Signed-off-by: discord9 <[email protected]> feat: dn gc limiter Signed-off-by: discord9 <[email protected]> rename Signed-off-by: discord9 <[email protected]> feat: gc scheduler retry with outdated regions Signed-off-by: discord9 <[email protected]> feat: use real object store purger Signed-off-by: discord9 <[email protected]> wip: add to metasrv Signed-off-by: discord9 <[email protected]> feat: add to metasrv Signed-off-by: discord9 <[email protected]> feat: datanode gc worker handler Signed-off-by: discord9 <[email protected]> fix: no partition col fix Signed-off-by: discord9 <[email protected]> fix: RegionId json deser workaround Signed-off-by: discord9 <[email protected]> fix: find access layer Signed-off-by: discord9 <[email protected]> fix: on host dn Signed-off-by: discord9 <[email protected]> fix: stat dedup Signed-off-by: discord9 <[email protected]> refactor: rm load-based Signed-off-by: discord9 <[email protected]> chore: aft rebase fix Signed-off-by: discord9 <[email protected]> feat: not full scan Signed-off-by: discord9 <[email protected]> chore: after rebase fix Signed-off-by: discord9 <[email protected]> feat: clean tracker Signed-off-by: discord9 <[email protected]> after rebase fix Signed-off-by: discord9 <[email protected]> clippy Signed-off-by: discord9 <[email protected]> refactor: split gc scheduler Signed-off-by: discord9 <[email protected]> feat: smaller linger time Signed-off-by: discord9 <[email protected]> feat: parallel region gc instr Signed-off-by: discord9 <[email protected]> chore: rename Signed-off-by: discord9 <[email protected]> chore: rename Signed-off-by: discord9 <[email protected]> enable is false Signed-off-by: discord9 <[email protected]> feat: update removed files precisely Signed-off-by: discord9 <[email protected]> all default to false&use local file purger Signed-off-by: discord9 <[email protected]> feat: not evict if gc enabled Signed-off-by: discord9 <[email protected]> per review Signed-off-by: discord9 <[email protected]> fix: pass gc config in mito&test: after truncate gc Signed-off-by: discord9 <[email protected]> WIP: one more test Signed-off-by: discord9 <[email protected]> test: basic compact Signed-off-by: discord9 <[email protected]> test: compact with ref Signed-off-by: discord9 <[email protected]> refactor: for easier mock Signed-off-by: discord9 <[email protected]> docs: explain race condition Signed-off-by: discord9 <[email protected]> feat: gc region procedure Signed-off-by: discord9 <[email protected]> refactor: ctx send gc/ref instr with procedure Signed-off-by: discord9 <[email protected]> fix: config deser to default Signed-off-by: discord9 <[email protected]> refactor: gc report Signed-off-by: discord9 <[email protected]> wip: async index file rm Signed-off-by: discord9 <[email protected]> fixme? Signed-off-by: discord9 <[email protected]> typo Signed-off-by: discord9 <[email protected]> more ut Signed-off-by: discord9 <[email protected]> test: more mock test Signed-off-by: discord9 <[email protected]> more Signed-off-by: discord9 <[email protected]> refactor: split mock test Signed-off-by: discord9 <[email protected]> clippy Signed-off-by: discord9 <[email protected]> refactor: rm stuff Signed-off-by: discord9 <[email protected]> test: mock add gc report per region Signed-off-by: discord9 <[email protected]> fix: stricter table failure condition Signed-off-by: discord9 <[email protected]> sutff Signed-off-by: discord9 <[email protected]> feat: can do different table gc same time&more todos Signed-off-by: discord9 <[email protected]> after rebase check Signed-off-by: discord9 <[email protected]>

Signed-off-by: discord9 <[email protected]>

github-actions bot added size/M docs-not-required This change does not impact docs. labels Sep 17, 2025

discord9 force-pushed the gc_metasrv_trigger branch 3 times, most recently from 9f76fb4 to 6bfa276 Compare September 18, 2025 11:04

github-actions bot added size/L and removed size/M labels Sep 18, 2025

discord9 force-pushed the gc_metasrv_trigger branch from 3b5e42f to 6bbe601 Compare September 19, 2025 07:04

discord9 changed the title ~~feat: gc worker metasrv trigger~~ feat: gc worker metasrv scheduler Sep 19, 2025

discord9 force-pushed the gc_metasrv_trigger branch 4 times, most recently from 4ac8dda to 3fa76b6 Compare October 15, 2025 03:09

discord9 force-pushed the gc_metasrv_trigger branch 3 times, most recently from 92dc253 to ed00d04 Compare October 22, 2025 09:01

github-actions bot added size/XL and removed size/L labels Oct 22, 2025

discord9 force-pushed the gc_metasrv_trigger branch 3 times, most recently from 162f2ed to 6a80190 Compare October 29, 2025 03:54

github-actions bot added size/L and removed size/XL labels Oct 29, 2025

discord9 force-pushed the gc_metasrv_trigger branch from 3f0b058 to 140cc73 Compare October 30, 2025 10:57

discord9 marked this pull request as ready for review October 30, 2025 12:25

discord9 requested review from a team, MichaelScofield, evenyag, v0y4g3r and waynexia as code owners October 30, 2025 12:25

MichaelScofield reviewed Oct 31, 2025

View reviewed changes

src/meta-srv/src/gc/mailbox.rs Outdated Show resolved Hide resolved

src/meta-srv/src/gc/options.rs Show resolved Hide resolved

src/mito2/src/gc.rs Outdated Show resolved Hide resolved

fengjiachun reviewed Nov 3, 2025

View reviewed changes

src/mito2/src/gc.rs Outdated Show resolved Hide resolved

evenyag requested review from MichaelScofield and fengjiachun November 5, 2025 08:04

killme2008 requested a review from Copilot November 5, 2025 08:15

Copilot AI reviewed Nov 5, 2025

View reviewed changes

discord9 force-pushed the gc_metasrv_trigger branch from 966b007 to db5dcc8 Compare November 5, 2025 08:44

killme2008 reviewed Nov 5, 2025

View reviewed changes

github-actions bot added documentation size/XL and removed size/L labels Nov 5, 2025

discord9 force-pushed the gc_metasrv_trigger branch from a4c1e27 to 25fb1c3 Compare November 7, 2025 06:23

discord9 marked this pull request as draft November 7, 2025 06:32

discord9 force-pushed the gc_metasrv_trigger branch from 25fb1c3 to 7bd7a3c Compare November 7, 2025 08:48

github-actions bot added size/XXL and removed size/XL labels Nov 10, 2025

discord9 force-pushed the gc_metasrv_trigger branch from 72c85c2 to 4324eb9 Compare November 10, 2025 11:04

discord9 added 11 commits November 12, 2025 16:00

feat: gc worker only on local region

93da6c1

Signed-off-by: discord9 <[email protected]>

more check

ce18e82

Signed-off-by: discord9 <[email protected]>

chore: stuff

c2a6983

Signed-off-by: discord9 <[email protected]>

fix: ignore async index file for now

5e83fd6

Signed-off-by: discord9 <[email protected]>

fix: file removal rate calc

ecd2f34

Signed-off-by: discord9 <[email protected]>

chore: per review

76140ff

Signed-off-by: discord9 <[email protected]>

chore: per review

f6f55c8

Signed-off-by: discord9 <[email protected]>

clippy

63fb63f

Signed-off-by: discord9 <[email protected]>

chore

aee4eaf

Signed-off-by: discord9 <[email protected]>

chore

e7fd871

Signed-off-by: discord9 <[email protected]>

discord9 force-pushed the gc_metasrv_trigger branch from e9db6c6 to e7fd871 Compare November 13, 2025 06:18

		/// Count the number of files removed after the given timestamp. Also return the minimum
		/// timestamp of all removed files.

		// expect long running queries to be finished(or at least be able to notify it's using a deleted file) within a reasonable time
		lingering_time: Some(Duration::from_secs(60)),

feat: gc worker metasrv scheduler #6985

Are you sure you want to change the base?

feat: gc worker metasrv scheduler #6985

Uh oh!

Conversation

discord9 commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Refer to a related PR or issue link (optional)

What's changed and what's your intention?

PR Checklist

Uh oh!

MichaelScofield left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

killme2008 commented Nov 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

discord9 commented Sep 17, 2025 •

edited

Loading