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

Support tiering when file endpoints overlap #10961

Closed
wants to merge 4 commits into from

Conversation

ajkr
Copy link
Contributor

@ajkr ajkr commented Nov 17, 2022

Enabled output to penultimate level when file endpoints overlap. This is probably only possible when range tombstones span files. Otherwise the overlapping files would all be included in the penultimate level inputs thanks to our atomic compaction unit logic.

Also, corrected penultimate_output_range_type_, which is a minor fix as it appears only used for logging.

Test Plan: updated unit test

@facebook-github-bot
Copy link
Contributor

@ajkr has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@ajkr ajkr force-pushed the support-tiering-with-delrange branch from 88d05cd to 3bf7ce8 Compare November 20, 2022 06:11
@facebook-github-bot
Copy link
Contributor

@ajkr has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@ajkr has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@ajkr ajkr requested a review from cbi42 November 21, 2022 16:59
@facebook-github-bot
Copy link
Contributor

@ajkr has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@ajkr has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Copy link
Member

@cbi42 cbi42 left a comment

Choose a reason for hiding this comment

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

LGTM, left some minor comment and question.

}
auto* snap2 = db_->GetSnapshot();
assert(kNumKeysPerFile >= 5);
Copy link
Member

Choose a reason for hiding this comment

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

Curious what is this assert for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh it was to ensure the below range tombstones are non-overlapping and there is one spanning the middle L5 file's left endpoint, one fully contained within the L5 file, and one spanning the middle L5 file's right endpoint. In retrospect I think the only part that matters is overlapping the endpoints, which doesn't need this assert, so I'll remove it.

DestroyAndReopen(options);

// pass some time first, otherwise the first a few keys write time are going
// to be zero, and internally zero has special meaning: kUnknownSeqnoTime
dbfull()->TEST_WaitForPeridicTaskRun(
[&] { mock_clock_->MockSleepForSeconds(static_cast<int>(10)); });
[&] { mock_clock_->MockSleepForSeconds(static_cast<int>(kKeyPerSec)); });
Copy link
Member

Choose a reason for hiding this comment

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

nit: should it be kSecPerKey?

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, done.

@ajkr ajkr force-pushed the support-tiering-with-delrange branch from 926a1d7 to d54df47 Compare November 22, 2022 23:30
Copy link
Contributor Author

@ajkr ajkr 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 review!

}
auto* snap2 = db_->GetSnapshot();
assert(kNumKeysPerFile >= 5);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh it was to ensure the below range tombstones are non-overlapping and there is one spanning the middle L5 file's left endpoint, one fully contained within the L5 file, and one spanning the middle L5 file's right endpoint. In retrospect I think the only part that matters is overlapping the endpoints, which doesn't need this assert, so I'll remove it.

DestroyAndReopen(options);

// pass some time first, otherwise the first a few keys write time are going
// to be zero, and internally zero has special meaning: kUnknownSeqnoTime
dbfull()->TEST_WaitForPeridicTaskRun(
[&] { mock_clock_->MockSleepForSeconds(static_cast<int>(10)); });
[&] { mock_clock_->MockSleepForSeconds(static_cast<int>(kKeyPerSec)); });
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, done.

@facebook-github-bot
Copy link
Contributor

@ajkr has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@ajkr has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

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.

3 participants