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 to verify that the dynamic priority can be passed from compaction to FS #10088

Closed
wants to merge 33 commits into from

Conversation

gitbw95
Copy link
Contributor

@gitbw95 gitbw95 commented Jun 1, 2022

Summary:
Add unit tests to verify that the dynamic priority can be passed from compaction to FS. Compaction reads&writes and other DB reads&writes share the same read&write paths to FSRandomAccessFile or FSWritableFile, so a MockTestFileSystem is added to replace the default filesystem from Env to intercept and verify the io_priority. To prepare the compaction input files, use the default filesystem from Env. To test the io priority of the compaction reads and writes, db_options_.fs is set as MockTestFileSystem.

Test Plan:
Add unit tests.

@gitbw95 gitbw95 requested a review from anand1976 June 2, 2022 19:13
@gitbw95 gitbw95 changed the title [WIP] Add unit test to verify that the dynamic priority can be passed from compaction to FS Add unit test to verify that the dynamic priority can be passed from compaction to FS Jun 2, 2022
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

Copy link
Contributor

@anand1976 anand1976 left a comment

Choose a reason for hiding this comment

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

Overall LGTM, but there seems to be a lot of code duplication between CompactionJobIOPriorityTestBase and CompactionJobTestBase. Is there any way we can reuse most of the code for the overridden functions? AFAICT, the main difference is which table builder to use.

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@gitbw95
Copy link
Contributor Author

gitbw95 commented Jun 7, 2022

Overall LGTM, but there seems to be a lot of code duplication between CompactionJobIOPriorityTestBase and CompactionJobTestBase. Is there any way we can reuse most of the code for the overridden functions? AFAICT, the main difference is which table builder to use.

This comment is addressed in the latest commit.

@facebook-github-bot
Copy link
Contributor

@gitbw95 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