From 9586dcf1ce6959f1680e618ff8ec89e011dc4099 Mon Sep 17 00:00:00 2001 From: Yanqin Jin Date: Wed, 22 Jun 2022 08:26:38 -0700 Subject: [PATCH] Expose the initial logger creation error (#10223) Summary: https://github.com/facebook/rocksdb/issues/9984 changes the behavior of RocksDB: if logger creation failed during `SanitizeOptions()`, `DB::Open()` will fail. However, since `SanitizeOptions()` is called in `DBImpl::DBImpl()`, we cannot directly expose the error to caller without some additional work. This is a first version proposal which: - Adds a new member `init_logger_creation_s` to `DBImpl` to store the result of init logger creation - Checks the error during `DB::Open()` and return it to caller if non-ok This is not very ideal. We can alternatively move the logger creation logic out of the `SanitizeOptions()`. Since `SanitizeOptions()` is used in other places, we need to check whether this change breaks anything in case other callers of `SanitizeOptions()` assumes that a logger should be created. Pull Request resolved: https://github.com/facebook/rocksdb/pull/10223 Test Plan: make check Reviewed By: pdillinger Differential Revision: D37321717 Pulled By: riversand963 fbshipit-source-id: 58042358a86369d606549dd9938933dd47591c4b --- db/db_basic_test.cc | 2 +- db/db_impl/db_impl.cc | 7 ++++++- db/db_impl/db_impl.h | 7 +++++-- db/db_impl/db_impl_open.cc | 14 ++++++++++---- utilities/backup/backup_engine_test.cc | 2 +- 5 files changed, 23 insertions(+), 9 deletions(-) diff --git a/db/db_basic_test.cc b/db/db_basic_test.cc index 55258434e..55fff5d7b 100644 --- a/db/db_basic_test.cc +++ b/db/db_basic_test.cc @@ -4143,7 +4143,7 @@ TEST_F(DBBasicTest, FailOpenIfLoggerCreationFail) { Status s = TryReopen(options); ASSERT_EQ(nullptr, options.info_log); - ASSERT_TRUE(s.IsAborted()); + ASSERT_TRUE(s.IsIOError()); SyncPoint::GetInstance()->DisableProcessing(); SyncPoint::GetInstance()->ClearAllCallBacks(); diff --git a/db/db_impl/db_impl.cc b/db/db_impl/db_impl.cc index 1aa315efc..15be4dcd8 100644 --- a/db/db_impl/db_impl.cc +++ b/db/db_impl/db_impl.cc @@ -156,7 +156,9 @@ DBImpl::DBImpl(const DBOptions& options, const std::string& dbname, bool read_only) : dbname_(dbname), own_info_log_(options.info_log == nullptr), - initial_db_options_(SanitizeOptions(dbname, options, read_only)), + init_logger_creation_s_(), + initial_db_options_(SanitizeOptions(dbname, options, read_only, + &init_logger_creation_s_)), env_(initial_db_options_.env), io_tracer_(std::make_shared()), immutable_db_options_(initial_db_options_), @@ -747,6 +749,9 @@ Status DBImpl::CloseHelper() { Status DBImpl::CloseImpl() { return CloseHelper(); } DBImpl::~DBImpl() { + // TODO: remove this. + init_logger_creation_s_.PermitUncheckedError(); + InstrumentedMutexLock closing_lock_guard(&closing_mutex_); if (closed_) { return; diff --git a/db/db_impl/db_impl.h b/db/db_impl/db_impl.h index 6c0543f00..2008085fd 100644 --- a/db/db_impl/db_impl.h +++ b/db/db_impl/db_impl.h @@ -1248,6 +1248,7 @@ class DBImpl : public DB { std::unique_ptr versions_; // Flag to check whether we allocated and own the info log file bool own_info_log_; + Status init_logger_creation_s_; const DBOptions initial_db_options_; Env* const env_; std::shared_ptr io_tracer_; @@ -2600,10 +2601,12 @@ class GetWithTimestampReadCallback : public ReadCallback { }; extern Options SanitizeOptions(const std::string& db, const Options& src, - bool read_only = false); + bool read_only = false, + Status* logger_creation_s = nullptr); extern DBOptions SanitizeOptions(const std::string& db, const DBOptions& src, - bool read_only = false); + bool read_only = false, + Status* logger_creation_s = nullptr); extern CompressionType GetCompressionFlush( const ImmutableCFOptions& ioptions, diff --git a/db/db_impl/db_impl_open.cc b/db/db_impl/db_impl_open.cc index c7d677792..0bd773fbe 100644 --- a/db/db_impl/db_impl_open.cc +++ b/db/db_impl/db_impl_open.cc @@ -27,8 +27,9 @@ namespace ROCKSDB_NAMESPACE { Options SanitizeOptions(const std::string& dbname, const Options& src, - bool read_only) { - auto db_options = SanitizeOptions(dbname, DBOptions(src), read_only); + bool read_only, Status* logger_creation_s) { + auto db_options = + SanitizeOptions(dbname, DBOptions(src), read_only, logger_creation_s); ImmutableDBOptions immutable_db_options(db_options); auto cf_options = SanitizeOptions(immutable_db_options, ColumnFamilyOptions(src)); @@ -36,7 +37,7 @@ Options SanitizeOptions(const std::string& dbname, const Options& src, } DBOptions SanitizeOptions(const std::string& dbname, const DBOptions& src, - bool read_only) { + bool read_only, Status* logger_creation_s) { DBOptions result(src); if (result.env == nullptr) { @@ -59,6 +60,9 @@ DBOptions SanitizeOptions(const std::string& dbname, const DBOptions& src, if (!s.ok()) { // No place suitable for logging result.info_log = nullptr; + if (logger_creation_s) { + *logger_creation_s = s; + } } } @@ -1790,9 +1794,11 @@ Status DBImpl::Open(const DBOptions& db_options, const std::string& dbname, DBImpl* impl = new DBImpl(db_options, dbname, seq_per_batch, batch_per_txn); if (!impl->immutable_db_options_.info_log) { - s = Status::Aborted("Failed to create logger"); + s = impl->init_logger_creation_s_; delete impl; return s; + } else { + assert(impl->init_logger_creation_s_.ok()); } s = impl->env_->CreateDirIfMissing(impl->immutable_db_options_.GetWalDir()); if (s.ok()) { diff --git a/utilities/backup/backup_engine_test.cc b/utilities/backup/backup_engine_test.cc index 6e47af89f..3b939c5ec 100644 --- a/utilities/backup/backup_engine_test.cc +++ b/utilities/backup/backup_engine_test.cc @@ -3066,7 +3066,7 @@ TEST_F(BackupEngineTest, OpenBackupAsReadOnlyDB) { db = nullptr; // Now try opening read-write and make sure it fails, for safety. - ASSERT_TRUE(DB::Open(opts, name, &db).IsAborted()); + ASSERT_TRUE(DB::Open(opts, name, &db).IsIOError()); } TEST_F(BackupEngineTest, ProgressCallbackDuringBackup) {