From 79e004404e09c74d402d0b6d4604a48f1e2ca88d Mon Sep 17 00:00:00 2001 From: Amirsaman Memaripour Date: Thu, 7 May 2020 19:23:18 -0400 Subject: SERVER-48013 Use atomics to reduce contention in hasSeenKeys() This commit simplifies the concurrency control in KeysCollectionManager and removes the unnecessary invocation of shouldGossipLogicalTime() in appendRequiredFieldsToResponse(). --- src/mongo/db/keys_collection_manager.cpp | 13 ++-- src/mongo/db/keys_collection_manager.h | 6 +- .../s/commands/cluster_command_test_fixture.cpp | 4 ++ src/mongo/s/commands/strategy.cpp | 77 +++++++++++++--------- 4 files changed, 60 insertions(+), 40 deletions(-) diff --git a/src/mongo/db/keys_collection_manager.cpp b/src/mongo/db/keys_collection_manager.cpp index 87f02b68e64..8b40033705f 100644 --- a/src/mongo/db/keys_collection_manager.cpp +++ b/src/mongo/db/keys_collection_manager.cpp @@ -223,6 +223,8 @@ void KeysCollectionManager::PeriodicRunner::_doPeriodicRefresh(ServiceContext* s Milliseconds refreshInterval) { ThreadClient tc(threadName, service); + ON_BLOCK_EXIT([this]() mutable { _hasSeenKeys.store(false); }); + while (true) { unsigned errorCount = 0; @@ -251,10 +253,7 @@ void KeysCollectionManager::PeriodicRunner::_doPeriodicRefresh(ServiceContext* s const auto& latestKey = latestKeyStatusWith.getValue(); auto currentTime = LogicalClock::get(service)->getClusterTime(); - { - stdx::unique_lock lock(_mutex); - _hasSeenKeys = true; - } + _hasSeenKeys.store(true); nextWakeup = howMuchSleepNeedFor(currentTime, latestKey.getExpiresAt(), refreshInterval); @@ -345,16 +344,14 @@ void KeysCollectionManager::PeriodicRunner::stop() { } _inShutdown = true; - _hasSeenKeys = false; _refreshNeededCV.notify_all(); } _backgroundThread.join(); } -bool KeysCollectionManager::PeriodicRunner::hasSeenKeys() { - stdx::lock_guard lock(_mutex); - return _hasSeenKeys; +bool KeysCollectionManager::PeriodicRunner::hasSeenKeys() const noexcept { + return _hasSeenKeys.load(); } } // namespace mongo diff --git a/src/mongo/db/keys_collection_manager.h b/src/mongo/db/keys_collection_manager.h index 1131b7c3612..751de346ef0 100644 --- a/src/mongo/db/keys_collection_manager.h +++ b/src/mongo/db/keys_collection_manager.h @@ -37,6 +37,7 @@ #include "mongo/db/keys_collection_cache.h" #include "mongo/db/keys_collection_document.h" #include "mongo/db/keys_collection_manager_gen.h" +#include "mongo/platform/atomic_word.h" #include "mongo/platform/mutex.h" #include "mongo/stdx/thread.h" #include "mongo/util/concurrency/notification.h" @@ -162,13 +163,15 @@ private: /** * Returns true if keys have ever successfully been returned from the config server. */ - bool hasSeenKeys(); + bool hasSeenKeys() const noexcept; private: void _doPeriodicRefresh(ServiceContext* service, std::string threadName, Milliseconds refreshInterval); + AtomicWord _hasSeenKeys{false}; + // protects all the member variables below. Mutex _mutex = MONGO_MAKE_LATCH("PeriodicRunner::_mutex"); std::shared_ptr> _refreshRequest; @@ -177,7 +180,6 @@ private: stdx::thread _backgroundThread; std::shared_ptr _doRefresh; - bool _hasSeenKeys = false; bool _inShutdown = false; }; diff --git a/src/mongo/s/commands/cluster_command_test_fixture.cpp b/src/mongo/s/commands/cluster_command_test_fixture.cpp index f3fe69feda8..60575f966c9 100644 --- a/src/mongo/s/commands/cluster_command_test_fixture.cpp +++ b/src/mongo/s/commands/cluster_command_test_fixture.cpp @@ -106,6 +106,10 @@ void ClusterCommandTestFixture::expectReturnsError(ErrorCodes::Error code) { } DbResponse ClusterCommandTestFixture::runCommand(BSONObj cmd) { + // TODO SERVER-48142 should remove the following fail-point usage. + // Skip appending required fields in unit-tests + FailPointEnableBlock skipAppendingReqFields("allowSkippingAppendRequiredFieldsToResponse"); + // Create a new client/operation context per command auto client = getServiceContext()->makeClient("ClusterCmdClient"); auto opCtx = client->makeOperationContext(); diff --git a/src/mongo/s/commands/strategy.cpp b/src/mongo/s/commands/strategy.cpp index d53bec20d32..2334232a604 100644 --- a/src/mongo/s/commands/strategy.cpp +++ b/src/mongo/s/commands/strategy.cpp @@ -133,44 +133,61 @@ Status processCommandMetadata(OperationContext* opCtx, const BSONObj& cmdObj) { return logicalClock->advanceClusterTime(signedTime.getTime()); } +/** + * Invoking `shouldGossipLogicalTime()` is expected to always return "true" during normal execution. + * SERVER-48013 uses this property to avoid the cost of calling this function during normal + * execution. However, it might be desired to do the validation for test purposes (e.g., + * unit-tests). This fail-point allows going through a code path that does the check and quick + * returns from `appendRequiredFieldsToResponse()` if `shouldGossipLogicalTime()` returns "false". + * TODO SERVER-48142 should remove the following fail-point. + */ +MONGO_FAIL_POINT_DEFINE(allowSkippingAppendRequiredFieldsToResponse); + /** * Append required fields to command response. */ void appendRequiredFieldsToResponse(OperationContext* opCtx, BSONObjBuilder* responseBuilder) { VectorClock::get(opCtx)->gossipOut(responseBuilder, opCtx->getClient()->getSessionTags()); - auto validator = LogicalTimeValidator::get(opCtx); - if (validator->shouldGossipLogicalTime()) { - auto now = LogicalClock::get(opCtx)->getClusterTime(); - - // Add operationTime. - auto operationTime = OperationTimeTracker::get(opCtx)->getMaxOperationTime(); - if (operationTime != LogicalTime::kUninitialized) { - LOGV2_DEBUG(22764, - 5, - "Appending operationTime: {operationTime}", - "Appending operationTime", - "operationTime"_attr = operationTime.asTimestamp()); - responseBuilder->append(kOperationTime, operationTime.asTimestamp()); - } else if (now != LogicalTime::kUninitialized) { - // If we don't know the actual operation time, use the cluster time instead. This is - // safe but not optimal because we can always return a later operation time than actual. - LOGV2_DEBUG(22765, - 5, - "Appending clusterTime as operationTime {clusterTime}", - "Appending clusterTime as operationTime", - "clusterTime"_attr = now.asTimestamp()); - responseBuilder->append(kOperationTime, now.asTimestamp()); - } - // Add $clusterTime. - if (LogicalTimeValidator::isAuthorizedToAdvanceClock(opCtx)) { - SignedLogicalTime dummySignedTime(now, TimeProofService::TimeProof(), 0); - rpc::LogicalTimeMetadata(dummySignedTime).writeToMetadata(responseBuilder); - } else { - auto currentTime = validator->signLogicalTime(opCtx, now); - rpc::LogicalTimeMetadata(currentTime).writeToMetadata(responseBuilder); + // TODO SERVER-48142 should remove the following block. + if (MONGO_unlikely(allowSkippingAppendRequiredFieldsToResponse.shouldFail())) { + auto validator = LogicalTimeValidator::get(opCtx); + if (!validator->shouldGossipLogicalTime()) { + LOGV2_DEBUG(4801301, 3, "Skipped gossiping logical time"); + return; } } + + auto now = LogicalClock::get(opCtx)->getClusterTime(); + + // Add operationTime. + auto operationTime = OperationTimeTracker::get(opCtx)->getMaxOperationTime(); + if (operationTime != LogicalTime::kUninitialized) { + LOGV2_DEBUG(22764, + 5, + "Appending operationTime: {operationTime}", + "Appending operationTime", + "operationTime"_attr = operationTime.asTimestamp()); + responseBuilder->append(kOperationTime, operationTime.asTimestamp()); + } else if (now != LogicalTime::kUninitialized) { + // If we don't know the actual operation time, use the cluster time instead. This is + // safe but not optimal because we can always return a later operation time than actual. + LOGV2_DEBUG(22765, + 5, + "Appending clusterTime as operationTime {clusterTime}", + "Appending clusterTime as operationTime", + "clusterTime"_attr = now.asTimestamp()); + responseBuilder->append(kOperationTime, now.asTimestamp()); + } + + // Add $clusterTime. + if (LogicalTimeValidator::isAuthorizedToAdvanceClock(opCtx)) { + SignedLogicalTime dummySignedTime(now, TimeProofService::TimeProof(), 0); + rpc::LogicalTimeMetadata(dummySignedTime).writeToMetadata(responseBuilder); + } else { + auto currentTime = LogicalTimeValidator::get(opCtx)->signLogicalTime(opCtx, now); + rpc::LogicalTimeMetadata(currentTime).writeToMetadata(responseBuilder); + } } /** -- cgit v1.2.1