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

overlapping endpoint fixes in level compaction picker #2615

Closed
wants to merge 1 commit into from

Conversation

ajkr
Copy link
Contributor

@ajkr ajkr commented Jul 19, 2017

This diff addresses two problems. Both problems cause us to miss scheduling desirable compactions. One side effect is compaction picking can spam logs, as there's no delay after failed attempts to pick compactions.

  1. If a compaction pulled in a locked input-level file due to user-key overlap, we would not consider picking another file from the same input level.
  2. If a compaction pulled in a locked output-level file due to user-key overlap, we would not consider picking any other compaction on any level.

The code changes are dependent, which is why I solved both problems in a single diff.

  • Moved input-level ExpandInputsToCleanCut into the loop inside PickFileToCompact. This gives two benefits: (1) if it fails, we will try the next-largest file on the same input level; (2) we get the fully-expanded input-level key-range with which we can check for pending compactions in output level.
  • Added another call to ExpandInputsToCleanCut inside PickFileToCompact's to check for compaction conflicts in output level.
  • Deleted call to IsRangeInCompaction in PickFileToCompact, as ExpandInputsToCleanCut also correctly handles the case where original output-level files (i.e., ones not pulled in due to user-key overlap) are pending compaction.

Test Plan: unit tests for the two problems mentioned

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

Copy link
Contributor

@sagar0 sagar0 left a comment

Choose a reason for hiding this comment

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

seems reasonable to me.

@ajkr
Copy link
Contributor Author

ajkr commented Jul 20, 2017

thanks a lot for the fast review! :)

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