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: avoid sst compacat twice #154

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

crwen
Copy link
Contributor

@crwen crwen commented Sep 12, 2024

#151

  • Add a compact status to avoid sst compact twice
  • Modify LevelStream::new declaration to accept needed data only
  • remove range index returned by this_level_scopes and next_level_scopes

Self::next_level_scopes(version, &mut min, &mut max, level, &meet_scopes_l)?;

meet_scopes_ll.retain(|scope| compact_status.insert(scope.gen));
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this retain meet_scopes_l? Because there will be no new sstable in meet_scopes_ll because compaction will increase step by step at level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be meet_scopes_ll. Because we only care sstables that may be compacted in next compaction loop

Copy link
Contributor

Choose a reason for hiding this comment

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

The meet_scopes_ll of this compaction is the meet_scopes_l of the next compaction, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, meet_scopes_l also need filtering, but it needn't insert to compact_status. So the code may be like this:

meet_scopes_l.retain(|scope| !compact_status.contains(&scope.gen));
meet_scopes_ll.iter().for_each(|scope| {
    compact_status.insert(scope.gen);
});

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if compact_status requires both meet_scopes_ll to be inserted, new sstables are ignored here, and is it better to just insert new sstables?

@KKould
Copy link
Contributor

KKould commented Sep 12, 2024

It looks like the unit test is missing to verify that it is working properly

@KKould KKould added the enhancement New feature or request label Sep 12, 2024
@KKould KKould changed the title fix: sst compacat twice feat: sst compacat twice Sep 12, 2024
@KKould KKould changed the title feat: sst compacat twice feat: avoid sst compacat twice Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Blocked
Development

Successfully merging this pull request may close these issues.

2 participants