-
Notifications
You must be signed in to change notification settings - Fork 592
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
fix(storage): handle delete range in same epoch as write #12651
Conversation
Codecov Report
@@ Coverage Diff @@
## main #12651 +/- ##
==========================================
- Coverage 69.27% 69.26% -0.01%
==========================================
Files 1470 1470
Lines 241305 241297 -8
==========================================
- Hits 167162 167143 -19
- Misses 74143 74154 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 4 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGM. @Little-Wallace PTAL
} else { | ||
// If the range delete comes from the same epoch, convert value to Delete anyway. | ||
debug_assert_eq!(iter_key.epoch, earliest_range_delete_which_can_see_iter_key); | ||
value = HummockValue::Delete; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: should we follow the logic in L780-790 to update last_table_stats
as well?
I do not understand. |
Why log store can not filter put-key just like |
It is dangerous to keep the same version of put-key with overlapped delete-range in one sstable. Because all our design is based on there will be no key overlap with delete-ranges in the same file. |
After offline discussion, we decide to change to truncate the current epoch in the next epoch to avoid doing range delete on a key written in the same epoch. |
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
Fix #12650
Currently in the code of compaction runner and imm merge, we introduce an assumption that
iter_key.epoch < del_iter.earliest_delete_since(iter_key.epoch)
in adebug_assert
. However,del_iter.earliest_delete_since(iter_key.epoch)
returns an epoch that is>= iter_key.epoch
. Therefore, thedebug_assert
depends on the assumption that the==
case will never happen, which means that delete range will not delete a key written in the same epoch. However, in log store, we will write the log item to storage when the buffer is full, and after the log item is consumed by the sink, it will truncate the log with a range delete, and both operation happen in the same epoch, which breaks the assumption.In this PR, we change our code to handle the case when the epoch of iter key is the same as the delete range. Previous when
iter_key.epoch < del_iter.earliest_delete_since(iter_key.epoch)
, it writes an extra delete in the epoch of the range delete. In this PR, in the new case thatiter_key.epoch == del_iter.earliest_delete_since(iter_key.epoch)
, we turn the insert toHummockValue::Delete
anyway.Checklist
./risedev check
(or alias,./risedev c
)Documentation
Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.