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

[Remote Compaction] Set Options File Number for CompactionInput under Mutex Lock #13394

Conversation

jaykorean
Copy link
Contributor

@jaykorean jaykorean commented Feb 12, 2025

Summary

Options File Number to be read by remote worker is part of the CompactionServiceInput. We've been setting this in ProcessKeyValueCompactionWithCompactionService() while the db_mutex is not held. This needs to be accessed while the mutex is held. The value can change as part of SetOptions() -> RenameTempFileToOptionsFile() as in following.

rocksdb/db/db_impl/db_impl.cc

Lines 5595 to 5596 in e697219

InstrumentedMutexLock l(&mutex_);
versions_->options_file_number_ = options_file_number;

Keep this value in memory during CompactionJob::Prepare() which is called while the mutex is held, so that we can easily access this later without mutex when building the CompactionInput for the remote compaction.

Thanks to the crash test. This was surfaced after #13378 merged.

Test Plan

Unit Test

./compaction_service_test

Crash Test

COERCE_CONTEXT_SWITCH=1 COMPILE_WITH_TSAN=1 CC=clang-13 CXX=clang++-13 ROCKSDB_DISABLE_ALIGNED_NEW=1 USE_CLANG=1 make V=1 -j100 dbg
python3 -u tools/db_crashtest.py blackbox --enable_remote_compaction=1 

@jaykorean jaykorean marked this pull request as ready for review February 12, 2025 00:33
@facebook-github-bot
Copy link
Contributor

@jaykorean has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Copy link
Contributor

@jowlyzhang jowlyzhang left a comment

Choose a reason for hiding this comment

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

LGTM!

@facebook-github-bot
Copy link
Contributor

@jaykorean merged this pull request in a30c020.

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