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

fix DBTest.AutomaticConflictsWithManualCompaction #3375

Closed
wants to merge 2 commits into from

Conversation

ajkr
Copy link
Contributor

@ajkr ajkr commented Jan 17, 2018

After af92d4a, only exclusive manual compaction can have conflict. dc360df updated the conflict-checking test case accordingly. But we missed the point that exclusive manual compaction can only conflict with automatic compactions scheduled after it, since it waits on pending automatic compactions before it begins running.

This PR updates the test case to ensure the automatic compactions are scheduled after the manual compaction starts but before it finishes, thus ensuring a conflict. I also cleaned up the test case to use less space as I saw it cause out-of-space error on travis.

Test Plan:

  • make check -j64

@facebook-github-bot
Copy link
Contributor

@ajkr has updated the pull request.

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.

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, please make sure AutomaticConflictsWithManualCompaction passes before merge

@facebook-github-bot
Copy link
Contributor

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

@ajkr
Copy link
Contributor Author

ajkr commented Jan 17, 2018

This uncovered another test failure: ColumnFamilyTest.SameCFManualAutomaticConflict. Fortunately, it was testing old behavior that we intentionally deprecated. It tested that a picked, unscheduled, non-exclusive manual compaction prevented automatic compactions from starting. Not sure how that ever made sense, but anyways it's gone now.

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.

@yiwu-arbug
Copy link
Contributor

@ajkr Thanks much! I didn't realize "exclusive manual compaction can only conflict with automatic compactions scheduled after it".

@ajkr
Copy link
Contributor Author

ajkr commented Jan 17, 2018

landing as it passed on phabricator

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.

amytai pushed a commit to amytai/rocksdb that referenced this pull request Mar 9, 2018
Summary:
After af92d4a, only exclusive manual compaction can have conflict. dc360df updated the conflict-checking test case accordingly. But we missed the point that exclusive manual compaction can only conflict with automatic compactions scheduled after it, since it waits on pending automatic compactions before it begins running.

This PR updates the test case to ensure the automatic compactions are scheduled after the manual compaction starts but before it finishes, thus ensuring a conflict. I also cleaned up the test case to use less space as I saw it cause out-of-space error on travis.
Closes facebook#3375

Differential Revision: D6735162

Pulled By: ajkr

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

None yet

4 participants