feat(storage): use exist_table_id to filter exist key when compaction #3038
Conversation
This file contains 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
hzxa21
reviewed
Jun 7, 2022
proto/hummock.proto
Outdated
| @@ -140,6 +140,9 @@ message CompactTask { | |||
| repeated common.ParallelUnitMapping vnode_mappings = 11; | |||
| // compaction group the task belongs to | |||
| uint64 compaction_group_id = 12; | |||
|
|
|||
| // exist_table_id for compaction drop key | |||
| repeated uint32 exist_table_id = 13; | |||
There was a problem hiding this comment.
nits: exist_table_id -> existing_table_ids
There was a problem hiding this comment.
fix it.
and i add some test below
- check drop all keys without existing_table_id_set, we can see after compaction, no sstable produce
- check to compaction diffence keyspace (table_id prefix) with existing_table_id_set, we can see that the key which have drop_table_id prefix has been drop, and the other prefix (with exist_table_id prefix) remain. the test check all keys by hummock_scan, to avoid unexpectd data lost
- local compaction logic and workflow not change , i think we test it on above unit test and e2e
hzxa21
reviewed
Jun 7, 2022
src/storage/src/hummock/compactor.rs
Outdated
Comment on lines
783
to
784
| // iter.next().await?; | ||
| // continue; |
hzxa21
reviewed
Jun 7, 2022
src/storage/src/hummock/compactor.rs
Outdated
| @@ -160,6 +160,7 @@ impl Compactor { | |||
| // VNode mappings are not required when compacting shared buffer to L0 | |||
| vnode_mappings: vec![], | |||
| compaction_group_id: StaticCompactionGroupId::StateDefault.into(), | |||
| exist_table_id: vec![], | |||
There was a problem hiding this comment.
This will cause shared buffer compaction producing empty SSTs. We should disable the check for dropped table ids for shared buffer compaction.
There was a problem hiding this comment.
Ok, i think that we can distinguish shared_buffer_compation and normal_compaction by using strategy like CompactionFilter (next commit)
Li0k
force-pushed
the
li0k/drop_mv_from_storage
branch
4 times, most recently
from
818fec6 to
244f571
Compare
Li0k
force-pushed
the
li0k/drop_mv_from_storage
branch
from
244f571 to
4b39ef2
Compare
Li0k
changed the title
Codecov Report
@@ Coverage Diff @@
## main #3038 +/- ##
==========================================
+ Coverage 73.49% 73.57% +0.07%
==========================================
Files 733 733
Lines 99536 99784 +248
==========================================
+ Hits 73157 73412 +255
+ Misses 26379 26372 -7
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
Li0k
force-pushed
the
li0k/drop_mv_from_storage
branch
from
b046871 to
8980df5
Compare
hzxa21
reviewed
Jun 8, 2022
src/storage/src/hummock/compactor.rs
Outdated
| } | ||
| } | ||
|
|
||
| // in our design, frontend avoid to access keys which had be deleted, so we dont need to | ||
| // consider the epoch when the compaction_filter match (it means that mv had drop) | ||
| if !compaction_filter.filter(iter_key) { |
There was a problem hiding this comment.
Suggested change
| if !compaction_filter.filter(iter_key) { | |
| if !drop && !compaction_filter.filter(iter_key) { |
2 tasks
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.
What's changed and what's your intention?
To support clean expired key when compaction
Please explain IN DETAIL what the changes are in this PR and why they are needed:
Checklist
Refer to a related PR or issue link (optional)