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

Fix MultiGet range deletion handling and a memory leak #10513

Closed
wants to merge 1 commit into from
Closed

Fix MultiGet range deletion handling and a memory leak #10513

wants to merge 1 commit into from

Conversation

anand1976
Copy link
Contributor

This PR fixes 2 bugs introduced in #10432 -

  1. If the bloom filter returned a negative result for all MultiGet keys in a file, the range tombstones in that file were being ignored, resulting in incorrect results if those tombstones covered a key in a higher level.
  2. If all the keys in a file were filtered out in TableCache::MultiGetFilter, the table cache handle was not being released.

Test plan:
Add a new unit test that fails without this fix

akankshamahajan15
akankshamahajan15 approved these changes Aug 9, 2022
View reviewed changes
Copy link
Contributor

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

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

@anand1976 anand1976 mentioned this pull request Aug 17, 2022
Closed
facebook-github-bot pushed a commit that referenced this pull request Aug 17, 2022
@facebook-github-bot
Fix range deletion handling in async MultiGet (#10534)
65814a4 
Summary:
The fix in #10513 was not complete w.r.t range deletion handling. It didn't handle the case where a file with a range tombstone covering a key also overlapped another key in the batch. In that case, ```mget_range``` would be non-empty. However, ```mget_range``` would only have the second key and, therefore, the first key would be skipped when iterating through the range tombstones in ```TableCache::MultiGet```.

Test plan -
1. Add a unit test
2. Run stress tests

Pull Request resolved: #10534

Reviewed By: akankshamahajan15

Differential Revision: D38773880

Pulled By: anand1976

fbshipit-source-id: dae491dbe52e18bbce5179b77b63f20771a66c00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akankshamahajan15 akankshamahajan15 akankshamahajan15 approved these changes

@siying siying Awaiting requested review from siying

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@anand1976 @facebook-github-bot @akankshamahajan15