Unschedule manual compaction from thread-pool queue #9625
Conversation
jay-zhuang
changed the title
|
@jay-zhuang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
jay-zhuang
force-pushed
the
unschedule_job
branch
from
bc5ded7
to
771a425
Compare
|
@jay-zhuang has updated the pull request. You must reimport the pull request before landing. |
1 similar comment
|
@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. |
|
|
||
| // Cancel manual compaction if there's any | ||
| if (HasPendingManualCompaction()) { | ||
| DisableManualCompaction(); |
| // other pending jobs. | ||
| if (manual.status.IsIncomplete() && | ||
| manual.status.subcode() == Status::SubCode::kManualCompactionPaused) { | ||
| MaybeScheduleFlushOrCompaction(); |
There was a problem hiding this comment.
Is it needed or here just in case since it's harmless and maybe useful?
There was a problem hiding this comment.
It could be useful in the following situation:
step0. compaction pool is full
step1. exclusive manual compaction is scheduled
step2. 4 new flushes happens -> unscheduled_compactions_ is bumped, but unable to schedule:
rocksdb/db/db_impl/db_impl_compaction_flush.cc
Lines 2441 to 2443 in d74468e
step3. Unschedule the manual compaction here, but the
unscheduled_compactions_ won't be scheduled until the next event to trigger MaybeScheduleFlushOrCompaction(). So I think we should explicitly try schedule it again.
There was a problem hiding this comment.
I updated comment to mention that's for scheduling potential auto compaction which was blocked by exclusive manual compaction.
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
jay-zhuang
force-pushed
the
unschedule_job
branch
from
f93acd1
to
951081c
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. |
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
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
Summary: #9625 didn't change the unschedule condition which was waiting for the background thread to clean-up the compaction. make sure we only unschedule the task when it's scheduled. Pull Request resolved: #9659 Reviewed By: ajkr Differential Revision: D34651820 Pulled By: jay-zhuang fbshipit-source-id: 23f42081b15ec8886cd81cbf131b116e0c74dc2f
Summary: #9625 didn't change the unschedule condition which was waiting for the background thread to clean-up the compaction. make sure we only unschedule the task when it's scheduled. Pull Request resolved: #9659 Reviewed By: ajkr Differential Revision: D34651820 Pulled By: jay-zhuang fbshipit-source-id: 23f42081b15ec8886cd81cbf131b116e0c74dc2f
Summary: #9625 didn't change the unschedule condition which was waiting for the background thread to clean-up the compaction. make sure we only unschedule the task when it's scheduled. Pull Request resolved: #9659 Reviewed By: ajkr Differential Revision: D34651820 Pulled By: jay-zhuang fbshipit-source-id: 23f42081b15ec8886cd81cbf131b116e0c74dc2f
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.Test Plan: unittest not hang