From 05dec0c7c70eb774740da7f1bc0df42a96f696c8 Mon Sep 17 00:00:00 2001 From: Yanqin Jin Date: Mon, 12 Nov 2018 12:22:10 -0800 Subject: [PATCH] Remove redundant member var and set options (#4631) Summary: In the past, both `DBImpl::atomic_flush_` and `DBImpl::immutable_db_options_.atomic_flush` exist. However, we fail to set `immutable_db_options_.atomic_flush`, but use `DBImpl::atomic_flush_` which is set correctly. This does not lead to incorrect behavior, but is a duplicate of information. Since `immutable_db_options_` is always there and has `atomic_flush`, we should use it as source of truth and remove `DBImpl::atomic_flush_`. Pull Request resolved: https://github.com/facebook/rocksdb/pull/4631 Differential Revision: D12928371 Pulled By: riversand963 fbshipit-source-id: f85a811959d3828aad4a3a1b05f71facf19c636d --- db/db_filesnapshot.cc | 2 +- db/db_impl.cc | 7 +++---- db/db_impl.h | 3 --- db/db_impl_compaction_flush.cc | 8 ++++---- db/db_impl_write.cc | 14 +++++++------- options/db_options.cc | 3 ++- options/options_helper.cc | 1 + 7 files changed, 18 insertions(+), 20 deletions(-) diff --git a/db/db_filesnapshot.cc b/db/db_filesnapshot.cc index e956b8a8a..ace0befb6 100644 --- a/db/db_filesnapshot.cc +++ b/db/db_filesnapshot.cc @@ -86,7 +86,7 @@ Status DBImpl::GetLiveFiles(std::vector& ret, if (flush_memtable) { // flush all dirty data to disk. Status status; - if (atomic_flush_) { + if (immutable_db_options_.atomic_flush) { autovector cfds; SelectColumnFamiliesForAtomicFlush(&cfds); mutex_.Unlock(); diff --git a/db/db_impl.cc b/db/db_impl.cc index 27c4be8a6..68f03d6c7 100644 --- a/db/db_impl.cc +++ b/db/db_impl.cc @@ -220,7 +220,6 @@ DBImpl::DBImpl(const DBOptions& options, const std::string& dbname, preserve_deletes_(options.preserve_deletes), closed_(false), error_handler_(this, immutable_db_options_, &mutex_), - atomic_flush_(options.atomic_flush), atomic_flush_commit_in_progress_(false) { // !batch_per_trx_ implies seq_per_batch_ because it is only unset for // WriteUnprepared, which should use seq_per_batch_. @@ -309,7 +308,7 @@ Status DBImpl::ResumeImpl() { FlushOptions flush_opts; // We allow flush to stall write since we are trying to resume from error. flush_opts.allow_write_stall = true; - if (atomic_flush_) { + if (immutable_db_options_.atomic_flush) { autovector cfds; SelectColumnFamiliesForAtomicFlush(&cfds); mutex_.Unlock(); @@ -401,7 +400,7 @@ void DBImpl::CancelAllBackgroundWork(bool wait) { if (!shutting_down_.load(std::memory_order_acquire) && has_unpersisted_data_.load(std::memory_order_relaxed) && !mutable_db_options_.avoid_flush_during_shutdown) { - if (atomic_flush_) { + if (immutable_db_options_.atomic_flush) { autovector cfds; SelectColumnFamiliesForAtomicFlush(&cfds); mutex_.Unlock(); @@ -3130,7 +3129,7 @@ Status DBImpl::IngestExternalFile( TEST_SYNC_POINT_CALLBACK("DBImpl::IngestExternalFile:NeedFlush", &need_flush); if (status.ok() && need_flush) { - if (atomic_flush_) { + if (immutable_db_options_.atomic_flush) { mutex_.Unlock(); autovector cfds; SelectColumnFamiliesForAtomicFlush(&cfds); diff --git a/db/db_impl.h b/db/db_impl.h index 71caff30c..ef108aaff 100644 --- a/db/db_impl.h +++ b/db/db_impl.h @@ -1611,9 +1611,6 @@ class DBImpl : public DB { ErrorHandler error_handler_; - // True if DB enables atomic flush. - bool atomic_flush_; - // True if the DB is committing atomic flush. // TODO (yanqin) the current impl assumes that the entire DB belongs to // a single atomic flush group. In the future we need to add a new class diff --git a/db/db_impl_compaction_flush.cc b/db/db_impl_compaction_flush.cc index dd292c090..2ca1659cb 100644 --- a/db/db_impl_compaction_flush.cc +++ b/db/db_impl_compaction_flush.cc @@ -211,7 +211,7 @@ Status DBImpl::FlushMemTableToOutputFile( Status DBImpl::FlushMemTablesToOutputFiles( const autovector& bg_flush_args, bool* made_progress, JobContext* job_context, LogBuffer* log_buffer) { - if (atomic_flush_) { + if (immutable_db_options_.atomic_flush) { return AtomicFlushMemTablesToOutputFiles(bg_flush_args, made_progress, job_context, log_buffer); } @@ -566,7 +566,7 @@ Status DBImpl::CompactRange(const CompactRangeOptions& options, if (flush_needed) { FlushOptions fo; fo.allow_write_stall = options.allow_write_stall; - if (atomic_flush_) { + if (immutable_db_options_.atomic_flush) { autovector cfds; SelectColumnFamiliesForAtomicFlush(&cfds); s = AtomicFlushMemTables(cfds, fo, FlushReason::kManualCompaction, @@ -1196,7 +1196,7 @@ Status DBImpl::Flush(const FlushOptions& flush_options, ROCKS_LOG_INFO(immutable_db_options_.info_log, "[%s] Manual flush start.", cfh->GetName().c_str()); Status s; - if (atomic_flush_) { + if (immutable_db_options_.atomic_flush) { s = AtomicFlushMemTables({cfh->cfd()}, flush_options, FlushReason::kManualFlush); } else { @@ -1212,7 +1212,7 @@ Status DBImpl::Flush(const FlushOptions& flush_options, Status DBImpl::Flush(const FlushOptions& flush_options, const std::vector& column_families) { Status s; - if (!atomic_flush_) { + if (!immutable_db_options_.atomic_flush) { for (auto cfh : column_families) { s = Flush(flush_options, cfh); if (!s.ok()) { diff --git a/db/db_impl_write.cc b/db/db_impl_write.cc index 3b2e4561d..6bfc98025 100644 --- a/db/db_impl_write.cc +++ b/db/db_impl_write.cc @@ -1029,7 +1029,7 @@ void DBImpl::SelectColumnFamiliesForAtomicFlush( // Assign sequence number for atomic flush. void DBImpl::AssignAtomicFlushSeq(const autovector& cfds) { - assert(atomic_flush_); + assert(immutable_db_options_.atomic_flush); auto seq = versions_->LastSequence(); for (auto cfd : cfds) { cfd->imm()->AssignAtomicFlushSeq(seq); @@ -1085,7 +1085,7 @@ Status DBImpl::SwitchWAL(WriteContext* write_context) { // no need to refcount because drop is happening in write thread, so can't // happen while we're in the write thread autovector cfds; - if (atomic_flush_) { + if (immutable_db_options_.atomic_flush) { SelectColumnFamiliesForAtomicFlush(&cfds); } else { for (auto cfd : *versions_->GetColumnFamilySet()) { @@ -1106,7 +1106,7 @@ Status DBImpl::SwitchWAL(WriteContext* write_context) { } } if (status.ok()) { - if (atomic_flush_) { + if (immutable_db_options_.atomic_flush) { AssignAtomicFlushSeq(cfds); } for (auto cfd : cfds) { @@ -1139,7 +1139,7 @@ Status DBImpl::HandleWriteBufferFull(WriteContext* write_context) { // no need to refcount because drop is happening in write thread, so can't // happen while we're in the write thread autovector cfds; - if (atomic_flush_) { + if (immutable_db_options_.atomic_flush) { SelectColumnFamiliesForAtomicFlush(&cfds); } else { ColumnFamilyData* cfd_picked = nullptr; @@ -1176,7 +1176,7 @@ Status DBImpl::HandleWriteBufferFull(WriteContext* write_context) { } } if (status.ok()) { - if (atomic_flush_) { + if (immutable_db_options_.atomic_flush) { AssignAtomicFlushSeq(cfds); } for (const auto cfd : cfds) { @@ -1307,7 +1307,7 @@ Status DBImpl::ThrottleLowPriWritesIfNeeded(const WriteOptions& write_options, Status DBImpl::ScheduleFlushes(WriteContext* context) { autovector cfds; - if (atomic_flush_) { + if (immutable_db_options_.atomic_flush) { SelectColumnFamiliesForAtomicFlush(&cfds); for (auto cfd : cfds) { cfd->Ref(); @@ -1333,7 +1333,7 @@ Status DBImpl::ScheduleFlushes(WriteContext* context) { } } if (status.ok()) { - if (atomic_flush_) { + if (immutable_db_options_.atomic_flush) { AssignAtomicFlushSeq(cfds); } FlushRequest flush_req; diff --git a/options/db_options.cc b/options/db_options.cc index fd3cdcccd..4e8134511 100644 --- a/options/db_options.cc +++ b/options/db_options.cc @@ -85,7 +85,8 @@ ImmutableDBOptions::ImmutableDBOptions(const DBOptions& options) allow_ingest_behind(options.allow_ingest_behind), preserve_deletes(options.preserve_deletes), two_write_queues(options.two_write_queues), - manual_wal_flush(options.manual_wal_flush) { + manual_wal_flush(options.manual_wal_flush), + atomic_flush(options.atomic_flush) { } void ImmutableDBOptions::Dump(Logger* log) const { diff --git a/options/options_helper.cc b/options/options_helper.cc index 64319253b..27a2252a0 100644 --- a/options/options_helper.cc +++ b/options/options_helper.cc @@ -126,6 +126,7 @@ DBOptions BuildDBOptions(const ImmutableDBOptions& immutable_db_options, immutable_db_options.preserve_deletes; options.two_write_queues = immutable_db_options.two_write_queues; options.manual_wal_flush = immutable_db_options.manual_wal_flush; + options.atomic_flush = immutable_db_options.atomic_flush; return options; }