[Feature] Support marking data files as shared in tablet metadata and skipping to delete shared data files in vacuum, leaving them for full gc to clean up #60140
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -510,6 +528,7 @@ void MetaFileBuilder::_sstable_meta_clean_after_alter_type() { | |||
| FileMetaPB file_meta; | |||
| file_meta.set_name(sstable.filename()); | |||
| file_meta.set_size(sstable.filesize()); | |||
| file_meta.set_shared(sstable.shared()); | |||
| _tablet_meta->mutable_orphan_files()->Add(std::move(file_meta)); | |||
| } | |||
| // Clear the SSTable metadata. | |||
There was a problem hiding this comment.
The most risky bug in this code is:
Incorrect index usage for shared_segments.
You can modify the code like this:
@@ -94,12 +94,15 @@ void MetaFileBuilder::apply_column_mode_partial_update(const TxnLogPB_OpWrite& op_write) {
for (int i = 0; i < op_write.rowset().segments_size(); ++i) {
FileMetaPB file_meta;
file_meta.set_name(op_write.rowset().segments(i));
- if (op_write.rowset().shared_segments_size() > 0) {
- file_meta.set_shared(op_write.rowset().shared_segments(i));
+ if (op_write.rowset().shared_segments_size() > i) { // Ensure index does not go out of bounds
+ file_meta.set_shared(op_write.rowset().shared_segments(i));
}
_tablet_meta->mutable_orphan_files()->Add(std::move(file_meta));
}
}This correction ensures you do not access indices outside the bounds of shared_segments, which would cause runtime errors.
| RETURN_IF_ERROR(deleter.delete_file(join_path(data_dir, f.name()))); | ||
| } | ||
| } | ||
| if (latest_metadata->sstable_meta().sstables_size() > 0) { | ||
| for (const auto& sst : latest_metadata->sstable_meta().sstables()) { | ||
| if (sst.shared()) { | ||
| continue; | ||
| } | ||
| RETURN_IF_ERROR(deleter.delete_file(join_path(data_dir, sst.filename()))); | ||
| } | ||
| } |
There was a problem hiding this comment.
The most risky bug in this code is:
Not correctly checking if a segment index exists in shared_segments causing possible undefined behavior or segmentation fault.
You can modify the code like this:
for (int i = 0; i < rowset.segments_size(); ++i) {
bool is_shared_segment = false;
if (rowset.shared_segments_size() > i) {
is_shared_segment = rowset.shared_segments(i);
}
if (is_shared_segment) {
continue;
}
RETURN_IF_ERROR(deleter.delete_file(join_path(data_dir, rowset.segments(i))));
}9f08e92 to
d953ec0
Compare
… skipping to delete shared data files in vacuum, leaving them for full gc to clean up Signed-off-by: xiangguangyxg <xiangguangyxg@gmail.com>
d953ec0 to
0e8b751
Compare
|
[Java-Extensions Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[FE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[BE Incremental Coverage Report]❌ fail : 38 / 54 (70.37%) file detail
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why I'm doing:
This is a preliminary work for tablet splitting and merging.
When an old tablet is split to two new tablets, the two new tablets will share all the data files (segment files, delvec files, cloud-native pk index files). These shared data files need to be marked as shared in tablet metadata and skipped to delete them in vacuum, leaving them for full gc to clean up.
What I'm doing:
Support marking data files as shared in tablet metadata and skipping to delete shared data files in vacuum, leaving them for full gc to clean up.
Fixes #59134
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check: