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.

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

feat(compaction): make compaction output be partitioned by parallel unit #2406

Merged
merged 11 commits into from
May 16, 2022

Conversation

soundOfDestiny
Copy link
Contributor

What's changed and what's your intention?

make compaction output be partitioned by parallel unit

Checklist

Refer to a related PR or issue link (optional)

@codecov
Copy link

codecov bot commented May 10, 2022

Codecov Report

Merging #2406 (92ba364) into main (a233b45) will increase coverage by 0.11%.
The diff coverage is 83.04%.

@@            Coverage Diff             @@
##             main    #2406      +/-   ##
==========================================
+ Coverage   71.75%   71.86%   +0.11%     
==========================================
  Files         681      678       -3     
  Lines       87931    87748     -183     
==========================================
- Hits        63092    63059      -33     
+ Misses      24839    24689     -150     
Flag Coverage Δ
rust 71.86% <83.04%> (+0.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/meta/src/hummock/compaction/mod.rs 82.87% <66.66%> (-0.78%) ⬇️
src/storage/src/hummock/compactor.rs 64.28% <68.51%> (+0.08%) ⬆️
src/storage/src/hummock/sstable/group_builder.rs 93.75% <70.00%> (+6.65%) ⬆️
...a/src/hummock/compaction/tier_compaction_picker.rs 97.34% <83.33%> (-0.18%) ⬇️
src/meta/src/hummock/compaction/level_selector.rs 97.08% <85.71%> (-0.30%) ⬇️
...rc/meta/src/hummock/compaction/overlap_strategy.rs 73.33% <86.66%> (+5.33%) ⬆️
src/storage/src/hummock/utils.rs 93.20% <93.75%> (-0.49%) ⬇️
src/storage/src/hummock/state_store.rs 81.72% <100.00%> (+9.11%) ⬆️
src/connector/src/filesystem/file_common.rs 80.35% <0.00%> (-0.45%) ⬇️
src/meta/src/barrier/mod.rs 67.88% <0.00%> (-0.34%) ⬇️
... and 8 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@soundOfDestiny soundOfDestiny force-pushed the zl_cpcout branch 5 times, most recently from d093654 to 04d0932 Compare May 10, 2022 08:19
as BoxedForwardHummockIterator);
};
}
for table_info in table_infos.into_iter().rev() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

SSTs in L1 overlap with one another after this change and we haven't implemented the logic to pass the vnode mapping to state store read interfaces. In this case, we will have a very lage merge iterator and I think this is why e2e times out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SSTs in L1 overlap with one another after this change and we haven't implemented the logic to pass the vnode mapping to state store read interfaces. In this case, we will have a very lage merge iterator and I think this is why e2e times out.

No. When I treated L1 as non-overlapping (i.e. not to ensure correctness), it still went time out

@soundOfDestiny soundOfDestiny force-pushed the zl_cpcout branch 2 times, most recently from d1fe0e7 to 40369c3 Compare May 11, 2022 12:32
Copy link
Collaborator

@hzxa21 hzxa21 left a comment

Choose a reason for hiding this comment

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

LGTM

@hzxa21
Copy link
Collaborator

hzxa21 commented May 16, 2022

Please double check e2e timeout issue will not be re-introduced in this PR before mering.

@soundOfDestiny
Copy link
Contributor Author

Please double check e2e timeout issue will not be re-introduced in this PR before mering.

ok. let me rebase main

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants