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

Add unit test for default temperature #11722

Closed
wants to merge 1 commit into from

Conversation

jowlyzhang
Copy link
Contributor

@jowlyzhang jowlyzhang commented Aug 18, 2023

This piggy back the existing last level file temperature statistics test to test the default temperature becoming effective.

While adding this unit test, I found that the approach to swap out and use default temperature in VersionBuilder::LoadTableHandlers will miss the L0 files created from flush, and only work for existing SST files, SST files created by compaction. So this PR moves that logic to TableCache::GetTableReader.

Test Plan:

./db_test2 --gtest_filter="*LastLevelStatistics*"
make all check

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@jowlyzhang jowlyzhang changed the title add unit test for default temp Add unit test for default temperature Aug 19, 2023
Copy link
Contributor

@pdillinger pdillinger left a comment

Choose a reason for hiding this comment

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

LGTM, thanks. Probably patch this against 8.6 for the internal user

@pdillinger
Copy link
Contributor

Looks like re-import is needed

@jowlyzhang
Copy link
Contributor Author

LGTM, thanks. Probably patch this against 8.6 for the internal user

Luckily, looks like Anand hasn't cut 8.6 yet, I will reach out to him for a favor to wait on this one. Thanks for the quick review!

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@jowlyzhang
Copy link
Contributor Author

LGTM, thanks. Probably patch this against 8.6 for the internal user

Luckily, looks like Anand hasn't cut 8.6 yet, I will reach out to him for a favor to wait on this one. Thanks for the quick review!

Sorry that this is mis info, I will patch this.

@facebook-github-bot
Copy link
Contributor

@jowlyzhang merged this pull request in 03a7441.

@jowlyzhang jowlyzhang deleted the ut_default_temp branch August 30, 2023 18:01
jowlyzhang added a commit that referenced this pull request Sep 13, 2023
Summary:
This piggy back the existing last level file temperature statistics test to test the default temperature becoming effective.

While adding this unit test, I found that the approach to swap out and use default temperature in `VersionBuilder::LoadTableHandlers` will miss the L0 files created from flush, and only work for existing SST files, SST files created by compaction. So this PR moves that logic to `TableCache::GetTableReader`.

Pull Request resolved: #11722

Test Plan:
```
./db_test2 --gtest_filter="*LastLevelStatistics*"
make all check
```

Reviewed By: pdillinger

Differential Revision: D48489171

Pulled By: jowlyzhang

fbshipit-source-id: ac29f7d484916f3218729594c5bb35c4f2979ac2
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