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

Jump to bottom

Fix: Avoid leveled compaction crash when recovering from manifest #63

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix: Avoid leveled compaction crash when recovering from manifest #63

wants to merge 3 commits into from

Conversation

YangchenYe323
Copy link
Contributor

Fixes #62

  • Add a test case to capture the error
    Below is what the test failure looks like on old version of the code:
--- STDERR:              mini-lsm tests::week2_day5::test_multiple_compacted_ssts_leveled ---
thread 'tests::week2_day5::test_multiple_compacted_ssts_leveled' panicked at mini-lsm/src/compact/leveled.rs:223:18:
called `Option::unwrap()` on a `None` value
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
  • Fix the error by moving the sorting logic outside of apply_compaction_result into trigger_compaction

skyzh
skyzh reviewed Mar 18, 2024
View reviewed changes
Copy link
Owner

@skyzh skyzh left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request and the fix! However, I really want the apply_compaction_result function keeps everything sorted, so that if we use that function, we always get a consistent state. I would rather opt in a design that apply_compaction_result takes a parameter in_recovery that controls whether to sort or not.

@skyzh
Copy link
Owner

skyzh commented Mar 18, 2024

Or we can include the start/end key in the manifest, which is what most storage engines are doing.

@YangchenYe323
Copy link
Contributor Author

@skyzh Which option do you prefer? I'm happy to implement either. Looks to me that adding a is_recovery flag is an easy change, whereas there are extra works in order to go the second route, e.g., you need to pass something like an Option<HashMap<usize, (KeyByte, KeyByte)>> to apply_compaction_result for it to use at recovery time. Does it align with your thinking? Personally I think adding a simple flag is fine. You don't need the leveled SSTs to be sorted at all times during recovery after all.

@skyzh
Copy link
Owner

skyzh commented Mar 18, 2024

Sounds good, let's have a recovery flag :) Take your time if you want to work on that, and feel free to ping me when you need review or help.

@YangchenYe323
Add in_recovery flag to apply_compaction_result
4766a39 
- Don't sort the SSTs inside `apply_compaction_result` if in recovery
@YangchenYe323
Copy link
Contributor Author

@skyzh Would you like to take another look? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skyzh skyzh Awaiting requested review from skyzh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Leveled compaction crashes when recovering from manifest
2 participants
@YangchenYe323 @skyzh