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

Update MultiGet to respect the strict_capacity_limit block cache option #13104

Closed
wants to merge 9 commits into from
Closed

Update MultiGet to respect the strict_capacity_limit block cache option #13104

wants to merge 9 commits into from

Conversation

archang19
Copy link
Contributor

@archang19 archang19 commented Oct 30, 2024 •
edited
Loading

Summary

There is a strict_capacity_limit option which imposes a hard memory limit on the block cache. When the block cache is enabled, every read request is serviced from the block cache. If the required block is missing, it is first inserted into the cache. If strict_capacity_limit is true and the limit has been reached, the Get and MultiGet requests should fail. However, currently this is not happening for MultiGet.

I updated MultiGet to explicitly check the returned status of MaybeReadBlockAndLoadToCache, so the status does not get overwritten later.

Thank you @anand1976 for the problem explanation.

Test Plan

Added unit test for both Get and MultiGet with a strict_capacity_limit set.

Before the change, half of my unit test cases failed https://github.com/facebook/rocksdb/actions/runs/11604597524/job/32313608085?pr=13104. After I added the check for the status returned by MaybeReadBlockAndLoadToCache, they all pass.

I also ran these tests manually (I had to run make clean before):

make -j64 block_based_table_reader_test COMPILE_WITH_ASAN=1 ASSERT_STATUS_CHECKED=1

 ./block_based_table_reader_test --gtest_filter="*StrictCapacityLimitReaderTest.Get*"
 ./block_based_table_reader_test --gtest_filter="*StrictCapacityLimitReaderTest.MultiGet*"

@archang19 archang19 force-pushed the strict-capacity-limit-multi-get branch 4 times, most recently from 7cdd8cb to 0370fdb Compare October 31, 2024 17:05
@archang19 archang19 changed the title [work in progress] Update MultiGet to respect the strict_capacity_limit block cache option Update MultiGet to respect the strict_capacity_limit block cache option Oct 31, 2024
anand1976
anand1976 reviewed Oct 31, 2024
View reviewed changes
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.

Thanks for the thorough test! LGTM.

table/block_based/block_based_table_reader_test.cc Outdated Show resolved Hide resolved
@archang19 archang19 force-pushed the strict-capacity-limit-multi-get branch from bf387a2 to 3e0ff07 Compare October 31, 2024 18:52
@archang19 archang19 marked this pull request as ready for review October 31, 2024 18:53
@archang19 archang19 force-pushed the strict-capacity-limit-multi-get branch from 3e0ff07 to a2f0a52 Compare October 31, 2024 21:42
@facebook-github-bot
Copy link
Contributor

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

@archang19 archang19 force-pushed the strict-capacity-limit-multi-get branch from 9e83077 to 97e727d Compare November 1, 2024 16:36
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

archang19
archang19 commented Nov 1, 2024
View reviewed changes
table/block_based/block_based_table_reader_sync_and_async.h
// block_entry value could be null if no block cache is present, i.e
// BlockBasedTableOptions::no_block_cache is true and no compressed
// block cache is configured. In that case, fall
// through and set up the block explicitly
if (block_entry->GetValue() != nullptr) {
s.PermitUncheckedError();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@anand1976 I saw that you added this code in #9968. I updated this PR to remove this line because now we have the !s.ok() block which also calls MarkChecked() inside Status

@facebook-github-bot
Copy link
Contributor

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

@archang19 archang19 force-pushed the strict-capacity-limit-multi-get branch from 97e727d to a6b0cb6 Compare November 1, 2024 17:25
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

anand1976
anand1976 approved these changes Nov 1, 2024
View reviewed changes
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.

LGTM

@facebook-github-bot
Copy link
Contributor

@archang19 merged this pull request in 7c98a2d.

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

@anand1976 anand1976 anand1976 approved these changes

Assignees
No one assigned
Labels
CLA Signed Merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@archang19 @facebook-github-bot @anand1976