[BugFix] Retain lake vacuum files by version interval across tablet reshard#76209
Conversation
|
@codex review |
Module-risk briefing for Codex reviewThis is review focus, not a finding list. Validate against the diff. High-confidence risks
Note: my attempt to post this directly via |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e91e6b31e2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
xiangguangyxg
force-pushed
the
fix-lake-vacuum-tolerate-missing-retain-version
branch
5 times, most recently
from
3e064c7 to
299c3f4
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 299c3f4d9c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
xiangguangyxg
left a comment
xiangguangyxg
left a comment
There was a problem hiding this comment.
Self-review (version-interval vacuum retention). The redesign is correct: keying retention on each file [create, remove) version interval removes the NotFound failure by construction and protects both shared and identical-tablet inherited files (reshard does not mark the latter shared). Retaining del files by their own version and stamping the orphan sites is thorough, and unset(0)->conservative-retain is the right fail-safe. Dependency: this needs #76208 for sstable.version() to be meaningful (remove_compacted_sst). Land #76208 first or together; if this lands alone, sstable orphans read version 0 -> conservatively retained (safe, just over-retains). Two inline notes below.
xiangguangyxg
force-pushed
the
fix-lake-vacuum-tolerate-missing-retain-version
branch
from
299c3f4 to
a2753bc
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…eshard
Auto-vacuum stalled and could lose snapshot data on shared-data
range-distribution partitions after a tablet split/merge when a cluster
snapshot pinned a pre-reshard version V:
- TabletRetainInfo::init read {tablet}_{V}.meta for each retained version to
build its protect set. On a reshard child, a pinned pre-reshard version V
predates the child's earliest metadata, so the read returned NotFound and
init failed. vacuum_tablet_metadata runs init before any deletion, so the
whole partition's vacuum RPC failed every cycle and expired metadata / data
files / txn logs accumulated for as long as the snapshot existed.
- Skipping the NotFound version, or only suppressing shared-file cleanup, is
unsafe: the incremental vacuum can delete a pre-reshard file the snapshot
still needs, and the reshard "identical tablet" path does not mark inherited
files shared, so a shared-flag-based guard misses them.
Decide retention by version interval instead of by reading the pinned version's
metadata. A garbage file is kept iff it was live at a retained version, i.e.
there is a retained version V with create_version <= V < remove_version, where
remove_version is the metadata version that moved it into compaction_inputs /
orphan_files.
- TabletRetainInfo now holds only the retained-version set (no metadata reads,
so the NotFound stall cannot occur) and answers
retained_by_version(create, remove) and contains_version(v).
- collect_garbage_files decides each garbage file's creation version:
- compaction_inputs segments share the rowset's version and are retained by
rowset.version(). Lake partial-segment compaction can carry older segments
into a higher-versioned rowset, but it is config-gated (off by default),
non-PK, and being retired in favor of parallel compaction, so it is
deliberately not special-cased;
- del files can be transferred by cloud-native PK compaction onto a
higher-versioned rowset, so they carry a new optional DelfileWithRowsetId
.version and are retained per file;
- orphan files carry a new optional FileMetaPB.version, stamped at each
orphaning site from the source object's own version.
An unset/0 version means "unknown -> retain conservatively", never a false
delete. Keyed on version rather than the per-file shared flag, this protects
both shared and exclusive/identical inherited files.
No user-facing SQL/config/API/metric change; adds two backward-compatible
optional fields to the storage metadata proto. Range distribution is a new,
new-tables-only feature with no on-disk compatibility burden.
Signed-off-by: xiangguangyxg <xiangguangyxg@gmail.com>
xiangguangyxg
force-pushed
the
fix-lake-vacuum-tolerate-missing-retain-version
branch
from
a2753bc to
0589b36
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
[Java-Extensions Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[FE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
|
[BE Incremental Coverage Report]✅ pass : 49 / 60 (81.67%) file detail
|
|
Tick the box to add this pull request to the merge queue (same as
|
|
@Mergifyio backport branch-4.1 |
xiangguangyxg
deleted the
fix-lake-vacuum-tolerate-missing-retain-version
branch
✅ Backports have been createdDetails
Cherry-pick of 2b7d675 has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally |
5 participants
Why I'm doing:
Shared-data range-distribution tables support tablet split/merge. When a cluster snapshot pins a
pre-reshard version
Vand the partition is then split/merged, the FE vacuum daemon enumerates thepartition's current (post-reshard child) tablets and ships the retained-version set (including
V)to the BE. A child tablet's earliest metadata is the reshard publish version (
> V), so it has no{child}_{V}.meta.TabletRetainInfo::initread{tablet}_{V}.metafor every retained version to build its protect set.For a child,
VreturnsNotFound, soinitfailed.vacuum_tablet_metadatacallsinitunderRETURN_IF_ERRORbefore any deletion, so the whole partition's vacuum RPC failed every cycle,lastSuccVacuumVersionnever advanced, and expired metadata / data files / txn logs accumulated foras long as the snapshot existed — an auto-vacuum stall / storage leak.
Naively skipping the
NotFoundversion is unsafe: the incremental vacuum can delete a pre-reshardsegment that the snapshot still needs (the grace-time clamp that protects automated snapshots does
not hold for a finished manual / external snapshot), and only-suppressing shared file cleanup is
also unsafe because the reshard "identical tablet" path does not mark its inherited files
shared,so they route through the regular deleter.
What I'm doing:
Decide vacuum retention by version interval instead of by reading the pinned version's metadata.
A garbage file must be kept iff it was live at a retained version, i.e. there is a retained version
Vwithcreate_version <= V < remove_version, whereremove_versionis the metadata version thatmoved the file into
compaction_inputs/orphan_files.TabletRetainInfonow holds only the retained-version set. It reads no per-version metadata, sothe
NotFoundfailure mode cannot occur by construction. It exposesretained_by_version(create_version, remove_version)(data-file liveness) andcontains_version(v)(tablet-metadata-file retention, unchanged). The old metadata read and the
contains_file/contains_rowsetname/id sets are removed.RowsetMetadataPB.version, preservedthrough split
CopyFromand compaction).so they carry a new optional
DelfileWithRowsetId.version, stamped at creation and retained perfile (independent of the containing rowset's version).
FileMetaPB.version, stamped from the source object's ownversion at each orphaning site (dcg
versions[i], delvecversion_to_filekey, sstablegeneration_version,idg entry
version; same-txn transient segment orphans are stamped with the metadata version beingbuilt). An unset/0 version means "unknown → retain conservatively", never a false delete.
collect_garbage_filesusesretained_by_version(candidate.version(), metadata.version()). Becausethe decision is keyed on version rather than the per-file
sharedflag, it correctly protects bothshared and exclusive/identical inherited files.
Note for reviewers: this changes the internal vacuum retain mechanism and adds two backward-compatible
optional fields (
FileMetaPB.versionandDelfileWithRowsetId.version) to the storage metadata proto.There is no user-facing SQL / config /
API / metric change, so this is classified as a bug fix (behavior change: No). Range distribution is a
new, new-tables-only feature with no on-disk compatibility burden.
Fixes #issue
What type of PR is this:
Does this PR entail a change in behavior?
Checklist:
Bugfix cherry-pick branch check: