From 6353f3613ac73d67bc064f7bbc81f949e6542838 Mon Sep 17 00:00:00 2001 From: Benety Goh Date: Tue, 13 Feb 2018 12:50:24 -0500 Subject: SERVER-32776 ReplicationProcess::getRollbackID() no longer reads from storage to refresh cached rollback id --- src/mongo/db/repl/repl_set_commands.cpp | 7 +------ src/mongo/db/repl/replication_coordinator_impl.cpp | 2 +- src/mongo/db/repl/replication_info.cpp | 6 +++--- src/mongo/db/repl/replication_process.cpp | 21 +++++---------------- src/mongo/db/repl/replication_process.h | 4 ++-- src/mongo/db/repl/replication_process_test.cpp | 10 +++++----- src/mongo/db/repl/rollback_impl_test.cpp | 4 ++-- src/mongo/db/s/session_catalog_migration_source.cpp | 7 ++----- 8 files changed, 21 insertions(+), 40 deletions(-) (limited to 'src') diff --git a/src/mongo/db/repl/repl_set_commands.cpp b/src/mongo/db/repl/repl_set_commands.cpp index f436cbf881c..1aa71ccfca7 100644 --- a/src/mongo/db/repl/repl_set_commands.cpp +++ b/src/mongo/db/repl/repl_set_commands.cpp @@ -171,12 +171,7 @@ public: if (!status.isOK()) return CommandHelpers::appendCommandStatus(result, status); - auto rbid = ReplicationProcess::get(opCtx)->getRollbackID(opCtx); - - // We should always have a Rollback ID since it is created at startup. - fassertStatusOK(40426, rbid.getStatus()); - - result.append("rbid", rbid.getValue()); + result.append("rbid", ReplicationProcess::get(opCtx)->getRollbackID()); return CommandHelpers::appendCommandStatus(result, Status::OK()); } } cmdReplSetRBID; diff --git a/src/mongo/db/repl/replication_coordinator_impl.cpp b/src/mongo/db/repl/replication_coordinator_impl.cpp index b13a6af2181..7c512259830 100644 --- a/src/mongo/db/repl/replication_coordinator_impl.cpp +++ b/src/mongo/db/repl/replication_coordinator_impl.cpp @@ -3194,7 +3194,7 @@ void ReplicationCoordinatorImpl::prepareReplMetadata(OperationContext* opCtx, // Avoid retrieving Rollback ID if we do not need it for _prepareOplogQueryMetadata_inlock(). int rbid = -1; if (hasOplogQueryMetadata) { - rbid = fassertStatusOK(40427, _replicationProcess->getRollbackID(opCtx)); + rbid = _replicationProcess->getRollbackID(); invariant(-1 != rbid); } diff --git a/src/mongo/db/repl/replication_info.cpp b/src/mongo/db/repl/replication_info.cpp index 72c73baa863..076c7e04a7c 100644 --- a/src/mongo/db/repl/replication_info.cpp +++ b/src/mongo/db/repl/replication_info.cpp @@ -172,9 +172,9 @@ public: BSONObjBuilder result; appendReplicationInfo(opCtx, result, level); - auto rbid = ReplicationProcess::get(opCtx)->getRollbackID(opCtx); - if (rbid.isOK()) { - result.append("rbid", rbid.getValue()); + auto rbid = ReplicationProcess::get(opCtx)->getRollbackID(); + if (ReplicationProcess::kUninitializedRollbackId != rbid) { + result.append("rbid", rbid); } return result.obj(); diff --git a/src/mongo/db/repl/replication_process.cpp b/src/mongo/db/repl/replication_process.cpp index 3e4b9cd7669..7f091a990a6 100644 --- a/src/mongo/db/repl/replication_process.cpp +++ b/src/mongo/db/repl/replication_process.cpp @@ -53,7 +53,6 @@ namespace { const auto getReplicationProcess = ServiceContext::declareDecoration>(); -const auto kRollbackNamespacePrefix = "local.system.rollback."_sd; } // namespace ReplicationProcess* ReplicationProcess::get(ServiceContext* service) { @@ -101,23 +100,13 @@ Status ReplicationProcess::refreshRollbackID(OperationContext* opCtx) { return Status::OK(); } -StatusWith ReplicationProcess::getRollbackID(OperationContext* opCtx) { +int ReplicationProcess::getRollbackID() const { stdx::lock_guard lock(_mutex); - - if (kUninitializedRollbackId != _rbid) { - return _rbid; - } - - // The _rbid, which caches the rollback ID persisted in the local.system.rollback.id collection, - // may be uninitialized because this is the first time we are retrieving the rollback ID. - auto rbidResult = _storageInterface->getRollbackID(opCtx); - if (!rbidResult.isOK()) { - return rbidResult; + if (kUninitializedRollbackId == _rbid) { + // This may happen when serverStatus is called by an internal client before we have a chance + // to read the rollback ID from storage. + warning() << "Rollback ID is not initialized yet."; } - log() << "Rollback ID is " << rbidResult.getValue(); - _rbid = rbidResult.getValue(); - - invariant(kUninitializedRollbackId != _rbid); return _rbid; } diff --git a/src/mongo/db/repl/replication_process.h b/src/mongo/db/repl/replication_process.h index ae155fe09d5..916b300d416 100644 --- a/src/mongo/db/repl/replication_process.h +++ b/src/mongo/db/repl/replication_process.h @@ -79,7 +79,7 @@ public: * Rollback ID is an increasing counter of how many rollbacks have occurred on this server. */ Status refreshRollbackID(OperationContext* opCtx); - StatusWith getRollbackID(OperationContext* opCtx); + int getRollbackID() const; Status initializeRollbackID(OperationContext* opCtx); Status incrementRollbackID(OperationContext* opCtx); @@ -102,7 +102,7 @@ private: // (M) Reads and writes guarded by _mutex. // Guards access to member variables. - stdx::mutex _mutex; + mutable stdx::mutex _mutex; // Used to access the storage layer. StorageInterface* const _storageInterface; // (R) diff --git a/src/mongo/db/repl/replication_process_test.cpp b/src/mongo/db/repl/replication_process_test.cpp index e031cd7551f..edf9a6645f5 100644 --- a/src/mongo/db/repl/replication_process_test.cpp +++ b/src/mongo/db/repl/replication_process_test.cpp @@ -97,11 +97,11 @@ TEST_F(ReplicationProcessTest, RollbackIDIncrementsBy1) { // We make no assumptions about the initial value of the rollback ID. ASSERT_OK(replicationProcess.initializeRollbackID(opCtx.get())); - int initRBID = unittest::assertGet(replicationProcess.getRollbackID(opCtx.get())); + int initRBID = replicationProcess.getRollbackID(); // Make sure the rollback ID is incremented by exactly 1. ASSERT_OK(replicationProcess.incrementRollbackID(opCtx.get())); - int rbid = unittest::assertGet(replicationProcess.getRollbackID(opCtx.get())); + int rbid = replicationProcess.getRollbackID(); ASSERT_EQ(rbid, initRBID + 1); } @@ -117,16 +117,16 @@ TEST_F(ReplicationProcessTest, RefreshRollbackIDResetsCachedValueFromStorage) { // We make no assumptions about the initial value of the rollback ID. ASSERT_OK(replicationProcess.initializeRollbackID(opCtx.get())); - int initRBID = unittest::assertGet(replicationProcess.getRollbackID(opCtx.get())); + int initRBID = replicationProcess.getRollbackID(); // Increment rollback ID on disk. Cached value should different from storage. int storageRBID = unittest::assertGet(_storageInterface->incrementRollbackID(opCtx.get())); ASSERT_EQUALS(storageRBID, initRBID + 1); - ASSERT_EQUALS(initRBID, unittest::assertGet(replicationProcess.getRollbackID(opCtx.get()))); + ASSERT_EQUALS(initRBID, replicationProcess.getRollbackID()); // Refresh cached value and check cached value against storage again. ASSERT_OK(replicationProcess.refreshRollbackID(opCtx.get())); - ASSERT_EQUALS(storageRBID, unittest::assertGet(replicationProcess.getRollbackID(opCtx.get()))); + ASSERT_EQUALS(storageRBID, replicationProcess.getRollbackID()); } } // namespace diff --git a/src/mongo/db/repl/rollback_impl_test.cpp b/src/mongo/db/repl/rollback_impl_test.cpp index 8ac13bb02eb..5b23f7278f3 100644 --- a/src/mongo/db/repl/rollback_impl_test.cpp +++ b/src/mongo/db/repl/rollback_impl_test.cpp @@ -266,13 +266,13 @@ TEST_F(RollbackImplTest, RollbackIncrementsRollbackID) { _localOplog->setOperations({op}); // Get the initial rollback id. - int initRollbackId = unittest::assertGet(_replicationProcess->getRollbackID(_opCtx.get())); + int initRollbackId = _replicationProcess->getRollbackID(); // Run rollback. ASSERT_OK(_rollback->runRollback(_opCtx.get())); // Check that the rollback id was incremented. - int newRollbackId = unittest::assertGet(_replicationProcess->getRollbackID(_opCtx.get())); + int newRollbackId = _replicationProcess->getRollbackID(); ASSERT_EQUALS(initRollbackId + 1, newRollbackId); } diff --git a/src/mongo/db/s/session_catalog_migration_source.cpp b/src/mongo/db/s/session_catalog_migration_source.cpp index e99913c1eeb..c3b96f0302f 100644 --- a/src/mongo/db/s/session_catalog_migration_source.cpp +++ b/src/mongo/db/s/session_catalog_migration_source.cpp @@ -113,9 +113,7 @@ repl::OplogEntry makeSentinelOplogEntry(OperationSessionInfo sessionInfo) { SessionCatalogMigrationSource::SessionCatalogMigrationSource(OperationContext* opCtx, NamespaceString ns) - : _ns(std::move(ns)), - _rollbackIdAtInit( - uassertStatusOK(repl::ReplicationProcess::get(opCtx)->getRollbackID(opCtx))) { + : _ns(std::move(ns)), _rollbackIdAtInit(repl::ReplicationProcess::get(opCtx)->getRollbackID()) { // Sort is not needed for correctness. This is just for making it easier to write deterministic // tests. Query query; @@ -325,8 +323,7 @@ repl::OplogEntry SessionCatalogMigrationSource::SessionOplogIterator::getNext( if (excep.code() == ErrorCodes::IncompleteTransactionHistory) { // Note: no need to check if in replicaSet mode because having an iterator implies // oplog exists. - auto rollbackId = - uassertStatusOK(repl::ReplicationProcess::get(opCtx)->getRollbackID(opCtx)); + auto rollbackId = repl::ReplicationProcess::get(opCtx)->getRollbackID(); uassert(40656, str::stream() << "rollback detected, rollbackId was " << _initialRollbackId -- cgit v1.2.1