Cancel manual compaction in thread-pool queue #9557
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jay-zhuang
force-pushed
the
disable_hang
branch
from
eed45b3
to
890eb27
Compare
jay-zhuang
force-pushed
the
disable_hang
branch
from
567f546
to
ab470d6
Compare
|
@jay-zhuang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
ajkr
approved these changes
Feb 15, 2022
| @@ -1879,6 +1879,20 @@ Status DBImpl::RunManualCompaction( | |||
| assert(!exclusive || !manual_conflict); | |||
| // Running either this or some other manual compaction | |||
| bg_cv_.Wait(); | |||
| if (manual_compaction_paused_ > 0 && !manual.done && | |||
| !manual.in_progress) { | |||
There was a problem hiding this comment.
Is my understanding right that for in_progress == true the cleanup / failure can be expected to arise in the background?
ajkr
reviewed
Feb 15, 2022
Comment on lines
1890
to
1891
| ca->prepicked_compaction->compaction->ReleaseCompactionFiles( | ||
| manual.status); |
There was a problem hiding this comment.
I can see the desire to release files as soon as possible, but would it also work to let the cleanup happen eventually when the background compaction finally runs? It kind of worries me to have a special foreground code path for releasing input files while letting rest of cleanup happen in the background.
There was a problem hiding this comment.
I think it makes sense to have the cleanup always happen in the background to avoid the cleanup conflict between background and foreground. (so technically we could remove the in_progress check above)
jay-zhuang
force-pushed
the
disable_hang
branch
from
b2e1d76
to
73bd0d4
Compare
|
@jay-zhuang has updated the pull request. You must reimport the pull request before landing. |
|
@jay-zhuang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
ajkr
approved these changes
Feb 16, 2022
jay-zhuang
added a commit
that referenced
this pull request
Feb 16, 2022
Summary: Fix `DisableManualCompaction()` has to wait scheduled manual compaction to start the execution to cancel the job. When a manual compaction in thread-pool queue is cancel, set the job is_canceled to true and clean the resource. Pull Request resolved: #9557 Test Plan: added unittest that will hang without the change Reviewed By: ajkr Differential Revision: D34214910 Pulled By: jay-zhuang fbshipit-source-id: 89dbaee78ddf26eb13ce862c2b15f4a098b36a78
jay-zhuang
added a commit
to jay-zhuang/rocksdb
that referenced
this pull request
Mar 2, 2022
Summary: PR facebook#9557 introduced a race condition between manual compaction foreground thread and background compaction thread. This PR adds the ability to really unschedule manual compaction from thread-pool queue by differentiate tag name for manual compaction and other tasks. Test Plan: Unittest not hang
facebook-github-bot
pushed a commit
that referenced
this pull request
Mar 2, 2022
Summary: PR #9557 introduced a race condition between manual compaction foreground thread and background compaction thread. This PR adds the ability to really unschedule manual compaction from thread-pool queue by differentiate tag name for manual compaction and other tasks. Also fix an issue that db `close()` didn't cancel the manual compaction thread. Pull Request resolved: #9625 Test Plan: unittest not hang Reviewed By: ajkr Differential Revision: D34410811 Pulled By: jay-zhuang fbshipit-source-id: cb14065eabb8cf1345fa042b5652d4f788c0c40c
ajkr
pushed a commit
that referenced
this pull request
Mar 3, 2022
Summary: PR #9557 introduced a race condition between manual compaction foreground thread and background compaction thread. This PR adds the ability to really unschedule manual compaction from thread-pool queue by differentiate tag name for manual compaction and other tasks. Also fix an issue that db `close()` didn't cancel the manual compaction thread. Pull Request resolved: #9625 Test Plan: unittest not hang Reviewed By: ajkr Differential Revision: D34410811 Pulled By: jay-zhuang fbshipit-source-id: cb14065eabb8cf1345fa042b5652d4f788c0c40c
ajkr
pushed a commit
that referenced
this pull request
Mar 7, 2022
Summary: PR #9557 introduced a race condition between manual compaction foreground thread and background compaction thread. This PR adds the ability to really unschedule manual compaction from thread-pool queue by differentiate tag name for manual compaction and other tasks. Also fix an issue that db `close()` didn't cancel the manual compaction thread. Pull Request resolved: #9625 Test Plan: unittest not hang Reviewed By: ajkr Differential Revision: D34410811 Pulled By: jay-zhuang fbshipit-source-id: cb14065eabb8cf1345fa042b5652d4f788c0c40c
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary: Fix
DisableManualCompaction()has to wait scheduled manual compaction to start the execution to cancel the job.When a manual compaction in thread-pool queue is cancel, set the job is_canceled to true and clean the resource.
Test Plan: added unittest that will hang without the change