Avoid opening table files and reading table properties under mutex #12879
Conversation
|
@cbi42 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| @@ -1595,11 +1599,6 @@ Status DBImpl::CompactFilesImpl( | |||
|
|
|||
| ReleaseFileNumberFromPendingOutputs(pending_outputs_inserted_elem); | |||
|
|
|||
| if (compaction_job_info != nullptr) { | |||
| BuildCompactionJobInfo(cfd, c.get(), s, compaction_job_stats, | |||
There was a problem hiding this comment.
There seems to be some info only known after compaction job is run e.g, output_table_properties, compaction_job_info->status, compaction_job_info->stats. Would it be too early to call this function compaction_job.Run()? Should we just release the lock for BuildCompactionJobInfo()?
There was a problem hiding this comment.
Good catch, updated to just release the mutex.
|
@cbi42 has updated the pull request. You must reimport the pull request before landing. |
|
@cbi42 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@cbi42 has updated the pull request. You must reimport the pull request before landing. |
|
@cbi42 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@cbi42 has updated the pull request. You must reimport the pull request before landing. |
db/compaction/compaction.h
Outdated
| const TablePropertiesCollection& GetInputTableProperties() const { | ||
| assert(!input_table_properties_.empty()); |
There was a problem hiding this comment.
Removed the assertion since failure of InitInputTableProperties() is not handled right now.
|
@cbi42 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: InitInputTableProperties() can open and do IOs and is called under mutex_. This PR removes it from FinalizeInputInfo(). It is now called in CompactionJob::Run() and BuildCompactionJobInfo() (called in NotifyOnCompactionBegin()) without holding mutex_.
Test plan: existing unit tests. Added assert in GetInputTableProperties() to ensure that input_table_properties_ is initialized whenever it's called.