Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(storage): fix correct_commit_ssts with sst table_ids #18414

Merged
merged 9 commits into from
Sep 5, 2024
+53 −36
Merged

fix(storage): fix correct_commit_ssts with sst table_ids #18414

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d0cc7e3
fix(storage): fix correct_commit_ssts with sst table_ids
Li0k Sep 5, 2024
8c7e44f
fix(storage): fix ut
Li0k Sep 5, 2024
22cb537
fix(storage): fix split_sst condition
Li0k Sep 5, 2024
be0b9ad
Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…
Li0k Sep 5, 2024
6ec7218
fix(storage): Ensure ordering of sstable_info.table_ids
Li0k Sep 5, 2024
dd1c495
Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…
Li0k Sep 5, 2024
2401f34
fix(storage): add assertion
Li0k Sep 5, 2024
e807d17
typo
Li0k Sep 5, 2024
d148d14
Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…
Li0k Sep 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions src/meta/src/hummock/manager/commit_epoch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,15 +426,17 @@ impl HummockManager {
for (mut sst, group_table_ids) in sst_to_cg_vec {
for (group_id, match_ids) in group_table_ids {
let group_members_table_ids = group_members_table_ids.get(&group_id).unwrap();
if match_ids
if sst
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems unnecessary and unclear to put this under the nested for loop. What we want to do here is to avoid SST split if all existing table ids of a sst belong to one and only one compaction group. Therefore, I suggest we put this check before L427 in the outer for loop. IIUC, we only need to check whether group_table_ids.len() == 1.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, do you think we need to modify table_ids and sst_size if group_table_ids.len() == 1 but table_ids in SstableInfo is a super set of group_member_table_ids (i.e. L407 is hit)

.sst_info
.table_ids
.iter()
.all(|id| group_members_table_ids.contains(&TableId::new(*id)))
{
commit_sstables
.entry(group_id)
.or_default()
.push(sst.sst_info.clone());
continue;
.push(sst.sst_info);
break;
}

let origin_sst_size = sst.sst_info.sst_size;
Expand Down
Loading