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

[DeleteFilesInRange] Preserve overlapping file endpoint invariant #2843

Closed
wants to merge 4 commits into from

Conversation

ajkr
Copy link
Contributor

@ajkr ajkr commented Sep 6, 2017

Fix for #2833.

  • In DeleteFilesInRange, use GetCleanInputsWithinInterval instead of GetOverlappingInputs to make sure we get a clean cut set of files to delete.
  • In GetCleanInputsWithinInterval, support nullptr as begin_key or end_key.
  • In GetOverlappingInputsRangeBinarySearch, move the assertion for non-empty range away from ExtendFileRangeWithinInterval, which should be allowed to return an empty range (via end_index < begin_index).

Test Plan:

  • new unit test

@facebook-github-bot
Copy link
Contributor

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

@ajkr
Copy link
Contributor Author

ajkr commented Sep 7, 2017

I think we should patch 5.7+ with this.

@zhangjinpeng87
Copy link
Contributor

zhangjinpeng87 commented Dec 6, 2017

@ajkr Can we merge this pr now?

@facebook-github-bot
Copy link
Contributor

@ajkr has updated the pull request. View: changes, changes since last import

Copy link
Contributor

@miasantreble miasantreble left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@ajkr is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@ajkr
Copy link
Contributor Author

ajkr commented Dec 6, 2017

Yes, thanks for reminder. Let me know if you need it backported to any branches.

@zhangjinpeng87
Copy link
Contributor

@ajkr We use delete range mix with delete files in range, so we need this pr. Currently we must fork ourself branch to achieve this approach.

@ajkr
Copy link
Contributor Author

ajkr commented Dec 7, 2017

But which rocksdb version do you currently use? 5.9.1?

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@facebook-github-bot
Copy link
Contributor

@ajkr has updated the pull request. View: changes, changes since last import

@zhangjinpeng87
Copy link
Contributor

zhangjinpeng87 commented Dec 7, 2017

@ajkr Currently we use 5.8.7 which we have tested for several months.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@ajkr is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@ajkr
Copy link
Contributor Author

ajkr commented Dec 7, 2017

Thanks @zhangjinpeng1987 . In that case I'll add the fix to 5.8 and 5.9 branches.

ajkr added a commit that referenced this pull request Dec 7, 2017
Summary:
Fix for #2833.

- In `DeleteFilesInRange`, use `GetCleanInputsWithinInterval` instead of `GetOverlappingInputs` to make sure we get a clean cut set of files to delete.
- In `GetCleanInputsWithinInterval`, support nullptr as `begin_key` or `end_key`.
- In `GetOverlappingInputsRangeBinarySearch`, move the assertion for non-empty range away from `ExtendFileRangeWithinInterval`, which should be allowed to return an empty range (via `end_index < begin_index`).
Closes #2843

Differential Revision: D5772387

Pulled By: ajkr

fbshipit-source-id: e554e8461823c6be82b21a9262a2da02b3957881
ajkr added a commit that referenced this pull request Dec 7, 2017
Summary:
Fix for #2833.

- In `DeleteFilesInRange`, use `GetCleanInputsWithinInterval` instead of `GetOverlappingInputs` to make sure we get a clean cut set of files to delete.
- In `GetCleanInputsWithinInterval`, support nullptr as `begin_key` or `end_key`.
- In `GetOverlappingInputsRangeBinarySearch`, move the assertion for non-empty range away from `ExtendFileRangeWithinInterval`, which should be allowed to return an empty range (via `end_index < begin_index`).
Closes #2843

Differential Revision: D5772387

Pulled By: ajkr

fbshipit-source-id: e554e8461823c6be82b21a9262a2da02b3957881
@ajkr
Copy link
Contributor Author

ajkr commented Dec 7, 2017

Tema pushed a commit that referenced this pull request Dec 7, 2017
Summary:
Fix for #2833.

- In `DeleteFilesInRange`, use `GetCleanInputsWithinInterval` instead of `GetOverlappingInputs` to make sure we get a clean cut set of files to delete.
- In `GetCleanInputsWithinInterval`, support nullptr as `begin_key` or `end_key`.
- In `GetOverlappingInputsRangeBinarySearch`, move the assertion for non-empty range away from `ExtendFileRangeWithinInterval`, which should be allowed to return an empty range (via `end_index < begin_index`).
Closes #2843

Differential Revision: D5772387

Pulled By: ajkr

fbshipit-source-id: e554e8461823c6be82b21a9262a2da02b3957881
zhangjinpeng87 pushed a commit to zhangjinpeng87/rocksdb that referenced this pull request Dec 11, 2017
Summary:
Fix for facebook#2833.

- In `DeleteFilesInRange`, use `GetCleanInputsWithinInterval` instead of `GetOverlappingInputs` to make sure we get a clean cut set of files to delete.
- In `GetCleanInputsWithinInterval`, support nullptr as `begin_key` or `end_key`.
- In `GetOverlappingInputsRangeBinarySearch`, move the assertion for non-empty range away from `ExtendFileRangeWithinInterval`, which should be allowed to return an empty range (via `end_index < begin_index`).
Closes facebook#2843

Differential Revision: D5772387

Pulled By: ajkr

fbshipit-source-id: e554e8461823c6be82b21a9262a2da02b3957881
huachaohuang pushed a commit to tikv/rocksdb that referenced this pull request Dec 11, 2017
Summary:
Fix for facebook#2833.

- In `DeleteFilesInRange`, use `GetCleanInputsWithinInterval` instead of `GetOverlappingInputs` to make sure we get a clean cut set of files to delete.
- In `GetCleanInputsWithinInterval`, support nullptr as `begin_key` or `end_key`.
- In `GetOverlappingInputsRangeBinarySearch`, move the assertion for non-empty range away from `ExtendFileRangeWithinInterval`, which should be allowed to return an empty range (via `end_index < begin_index`).
Closes facebook#2843

Differential Revision: D5772387

Pulled By: ajkr

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

4 participants