SERVER-45082 Utilize ConfigVersionAndTerm structure for comparison and string formatting

10 files changed, 49 insertions, 59 deletions

diff --git a/src/mongo/db/repl/repl_set_config.h b/src/mongo/db/repl/repl_set_config.h
index 23d2614adef..36fdb2d0e8e 100644
--- a/src/mongo/db/repl/repl_set_config.h
+++ b/src/mongo/db/repl/repl_set_config.h
@@ -94,9 +94,8 @@ public:
         return !(*this < rhs);
     }
 
-    // TODO (SERVER-45082): Implement string conversion.
     std::string toString() const {
-        return "";
+        return str::stream() << "{version: " << _version << ", term: " << _term << "}";
     };
 
     friend std::ostream& operator<<(std::ostream& out, const ConfigVersionAndTerm& cvt) {
diff --git a/src/mongo/db/repl/repl_set_config_checks.cpp b/src/mongo/db/repl/repl_set_config_checks.cpp
index 20eb82b5cec..19648e8319a 100644
--- a/src/mongo/db/repl/repl_set_config_checks.cpp
+++ b/src/mongo/db/repl/repl_set_config_checks.cpp
@@ -63,10 +63,10 @@ StatusWith<int> findSelfInConfig(ReplicationCoordinatorExternalState* externalSt
     }
     if (meConfigs.empty()) {
         return StatusWith<int>(ErrorCodes::NodeNotFound,
-                               str::stream() << "No host described in new configuration with term "
-                                             << newConfig.getConfigTerm() << " and version "
-                                             << newConfig.getConfigVersion() << " for replica set "
-                                             << newConfig.getReplSetName() << " maps to this node");
+                               str::stream() << "No host described in new configuration with "
+                                             << newConfig.getConfigVersionAndTerm().toString()
+                                             << " for replica set " << newConfig.getReplSetName()
+                                             << " maps to this node");
     }
     if (meConfigs.size() > 1) {
         str::stream message;
@@ -75,9 +75,9 @@ StatusWith<int> findSelfInConfig(ReplicationCoordinatorExternalState* externalSt
             message << ", " << meConfigs[i]->getHostAndPort().toString();
         }
         message << " and " << meConfigs.back()->getHostAndPort().toString()
-                << " all map to this node in new configuration with term "
-                << newConfig.getConfigTerm() << " and version " << newConfig.getConfigVersion()
-                << " for replica set " << newConfig.getReplSetName();
+                << " all map to this node in new configuration with "
+                << newConfig.getConfigVersionAndTerm().toString() << " for replica set "
+                << newConfig.getReplSetName();
         return StatusWith<int>(ErrorCodes::InvalidReplicaSetConfig, message);
     }
 
@@ -96,10 +96,9 @@ Status checkElectable(const ReplSetConfig& newConfig, int configIndex) {
         return Status(ErrorCodes::NodeNotElectable,
                       str::stream() << "This node, " << myConfig.getHostAndPort().toString()
                                     << ", with _id " << myConfig.getId()
-                                    << " is not electable under the new configuration with term "
-                                    << newConfig.getConfigTerm() << " and version "
-                                    << newConfig.getConfigVersion() << " for replica set "
-                                    << newConfig.getReplSetName());
+                                    << " is not electable under the new configuration with "
+                                    << newConfig.getConfigVersionAndTerm().toString()
+                                    << " for replica set " << newConfig.getReplSetName());
     }
     return Status::OK();
 }
diff --git a/src/mongo/db/repl/repl_set_config_test.cpp b/src/mongo/db/repl/repl_set_config_test.cpp
index bdb4770bfa2..506842f66a3 100644
--- a/src/mongo/db/repl/repl_set_config_test.cpp
+++ b/src/mongo/db/repl/repl_set_config_test.cpp
@@ -1735,6 +1735,13 @@ TEST(ReplSetConfig, ConfigVersionAndTermComparison) {
     ASSERT_GT(ConfigVersionAndTerm(2, -1), ConfigVersionAndTerm(1, 1));
     ASSERT_GT(ConfigVersionAndTerm(2, -1), ConfigVersionAndTerm(1, -1));
 }
+TEST(ReplSetConfig, ConfigVersionAndTermToString) {
+    ASSERT_EQ(ConfigVersionAndTerm(0, 1).toString(), "{version: 0, term: 1}");
+    ASSERT_EQ(ConfigVersionAndTerm(0, 2).toString(), "{version: 0, term: 2}");
+    ASSERT_EQ(ConfigVersionAndTerm(1, 1).toString(), "{version: 1, term: 1}");
+    ASSERT_EQ(ConfigVersionAndTerm(1, 2).toString(), "{version: 1, term: 2}");
+    ASSERT_EQ(ConfigVersionAndTerm(1, -1).toString(), "{version: 1, term: -1}");
+}
 }  // namespace
 }  // namespace repl
 }  // namespace mongo
diff --git a/src/mongo/db/repl/repl_set_request_votes_args.cpp b/src/mongo/db/repl/repl_set_request_votes_args.cpp
index 413fbcf0b4d..e72af47af64 100644
--- a/src/mongo/db/repl/repl_set_request_votes_args.cpp
+++ b/src/mongo/db/repl/repl_set_request_votes_args.cpp
@@ -129,6 +129,10 @@ long long ReplSetRequestVotesArgs::getConfigTerm() const {
     return _cfgTerm;
 }
 
+ConfigVersionAndTerm ReplSetRequestVotesArgs::getConfigVersionAndTerm() const {
+    return ConfigVersionAndTerm(_cfgVer, _cfgTerm);
+}
+
 OpTime ReplSetRequestVotesArgs::getLastDurableOpTime() const {
     return _lastDurableOpTime;
 }
diff --git a/src/mongo/db/repl/repl_set_request_votes_args.h b/src/mongo/db/repl/repl_set_request_votes_args.h
index f17f25a3bbd..c86429e79ed 100644
--- a/src/mongo/db/repl/repl_set_request_votes_args.h
+++ b/src/mongo/db/repl/repl_set_request_votes_args.h
@@ -32,6 +32,7 @@
 #include <string>
 
 #include "mongo/db/repl/optime.h"
+#include "mongo/db/repl/repl_set_config.h"
 
 namespace mongo {
 
@@ -48,6 +49,7 @@ public:
     long long getCandidateIndex() const;
     long long getConfigVersion() const;
     long long getConfigTerm() const;
+    ConfigVersionAndTerm getConfigVersionAndTerm() const;
     OpTime getLastDurableOpTime() const;
     bool isADryRun() const;
 
diff --git a/src/mongo/db/repl/replication_coordinator_impl.cpp b/src/mongo/db/repl/replication_coordinator_impl.cpp
index 5931e3f4f7a..f1db12180a2 100644
--- a/src/mongo/db/repl/replication_coordinator_impl.cpp
+++ b/src/mongo/db/repl/replication_coordinator_impl.cpp
@@ -2825,8 +2825,8 @@ void ReplicationCoordinatorImpl::_finishReplSetReconfig(OperationContext* opCtx,
     // we have already set our ReplicationCoordinatorImpl::_rsConfigState state to
     // "kConfigReconfiguring" which prevents new elections from happening.
     if (electionFinishedEvent) {
-        LOG(2) << "Waiting for election to complete before finishing reconfig to config with term "
-               << newConfig.getConfigTerm() << ", version " << newConfig.getConfigVersion();
+        LOG(2) << "Waiting for election to complete before finishing reconfig to config with "
+               << newConfig.getConfigVersionAndTerm();
         // Wait for the election to complete and the node's Role to be set to follower.
         _replExecutor->waitForEvent(electionFinishedEvent);
     }
diff --git a/src/mongo/db/repl/replication_coordinator_impl_heartbeat.cpp b/src/mongo/db/repl/replication_coordinator_impl_heartbeat.cpp
index a20e7d5b975..db9d264aa16 100644
--- a/src/mongo/db/repl/replication_coordinator_impl_heartbeat.cpp
+++ b/src/mongo/db/repl/replication_coordinator_impl_heartbeat.cpp
@@ -445,16 +445,14 @@ void ReplicationCoordinatorImpl::_scheduleHeartbeatReconfig_inlock(const ReplSet
     switch (_rsConfigState) {
         case kConfigUninitialized:
         case kConfigSteady:
-            LOG_FOR_HEARTBEATS(1) << "Received new config via heartbeat with term "
-                                  << newConfig.getConfigTerm() << ", version "
-                                  << newConfig.getConfigVersion();
+            LOG_FOR_HEARTBEATS(1) << "Received new config via heartbeat with "
+                                  << newConfig.getConfigVersionAndTerm();
             break;
         case kConfigInitiating:
         case kConfigReconfiguring:
         case kConfigHBReconfiguring:
-            LOG_FOR_HEARTBEATS(1) << "Ignoring new configuration with term "
-                                  << newConfig.getConfigTerm() << ", version "
-                                  << newConfig.getConfigVersion()
+            LOG_FOR_HEARTBEATS(1) << "Ignoring new configuration with "
+                                  << newConfig.getConfigVersionAndTerm()
                                   << " because already in the midst of a configuration process.";
             return;
         case kConfigPreStart:
@@ -468,9 +466,8 @@ void ReplicationCoordinatorImpl::_scheduleHeartbeatReconfig_inlock(const ReplSet
     invariant(!_rsConfig.isInitialized() ||
               _rsConfig.getConfigVersionAndTerm() < newConfig.getConfigVersionAndTerm());
     if (auto electionFinishedEvent = _cancelElectionIfNeeded_inlock()) {
-        LOG_FOR_HEARTBEATS(2) << "Rescheduling heartbeat reconfig to config with term "
-                              << newConfig.getConfigTerm() << ", version "
-                              << newConfig.getConfigVersion()
+        LOG_FOR_HEARTBEATS(2) << "Rescheduling heartbeat reconfig to config with "
+                              << newConfig.getConfigVersionAndTerm()
                               << " to be processed after election is cancelled.";
 
         _replExecutor
@@ -520,8 +517,7 @@ void ReplicationCoordinatorImpl::_heartbeatReconfigStore(
                      "it is invalid: "
                   << myIndex.getStatus();
     } else {
-        LOG_FOR_HEARTBEATS(2) << "Config with term " << newConfig.getConfigTerm() << ", version "
-                              << newConfig.getConfigVersion()
+        LOG_FOR_HEARTBEATS(2) << "Config with " << newConfig.getConfigVersionAndTerm()
                               << " validated for reconfig; persisting to disk.";
 
         auto opCtx = cc().makeOperationContext();
@@ -558,8 +554,7 @@ void ReplicationCoordinatorImpl::_heartbeatReconfigStore(
             shouldStartDataReplication = true;
         }
 
-        LOG_FOR_HEARTBEATS(2) << "New configuration with term " << newConfig.getConfigTerm()
-                              << ", version " << newConfig.getConfigVersion()
+        LOG_FOR_HEARTBEATS(2) << "New configuration with " << newConfig.getConfigVersionAndTerm()
                               << " persisted to local storage; installing new config in memory";
     }
 
@@ -602,8 +597,8 @@ void ReplicationCoordinatorImpl::_heartbeatReconfigFinish(
         stdx::lock_guard<Latch> lk(_mutex);
         if (auto electionFinishedEvent = _cancelElectionIfNeeded_inlock()) {
             LOG_FOR_HEARTBEATS(0)
-                << "Waiting for election to complete before finishing reconfig to config with term "
-                << newConfig.getConfigTerm() << ", version " << newConfig.getConfigVersion();
+                << "Waiting for election to complete before finishing reconfig to config with "
+                << newConfig.getConfigVersionAndTerm();
             // Wait for the election to complete and the node's Role to be set to follower.
             _replExecutor
                 ->onEvent(electionFinishedEvent,
diff --git a/src/mongo/db/repl/replication_coordinator_impl_test.cpp b/src/mongo/db/repl/replication_coordinator_impl_test.cpp
index df8663533ab..aefefa5177e 100644
--- a/src/mongo/db/repl/replication_coordinator_impl_test.cpp
+++ b/src/mongo/db/repl/replication_coordinator_impl_test.cpp
@@ -289,8 +289,7 @@ TEST_F(ReplCoordTest,
                                                                                 << "node2:12345"))),
                                                &result1);
     ASSERT_EQUALS(ErrorCodes::InvalidReplicaSetConfig, status);
-    ASSERT_STRING_CONTAINS(status.reason(),
-                           "is not electable under the new configuration with term");
+    ASSERT_STRING_CONTAINS(status.reason(), "is not electable under the new configuration with");
     ASSERT_FALSE(getExternalState()->threadsStarted());
 }
 
diff --git a/src/mongo/db/repl/topology_coordinator.cpp b/src/mongo/db/repl/topology_coordinator.cpp
index 2248dc2e450..832373164e9 100644
--- a/src/mongo/db/repl/topology_coordinator.cpp
+++ b/src/mongo/db/repl/topology_coordinator.cpp
@@ -2773,33 +2773,16 @@ void TopologyCoordinator::processReplSetRequestVotes(const ReplSetRequestVotesAr
         return;
     }
 
-    // If either config term is -1, ignore the config term entirely and compare config versions.
-    bool compareConfigTerms = args.getConfigTerm() != -1 && _rsConfig.getConfigTerm() != -1;
-
     if (args.getTerm() < _term) {
         response->setVoteGranted(false);
         response->setReason(str::stream() << "candidate's term ({}) is lower than mine ({})"_format(
                                 args.getTerm(), _term));
-    } else if (compareConfigTerms && args.getConfigTerm() < _rsConfig.getConfigTerm()) {
-        response->setVoteGranted(false);
-        response->setReason(str::stream()
-                            << "candidate's term in config(term, version): ({}, {}) is lower "
-                               "than mine ({}, {})"_format(args.getConfigTerm(),
-                                                           args.getConfigVersion(),
-                                                           _rsConfig.getConfigTerm(),
-                                                           _rsConfig.getConfigVersion()));
-    } else if ((!compareConfigTerms || args.getConfigTerm() == _rsConfig.getConfigTerm()) &&
-               args.getConfigVersion() < _rsConfig.getConfigVersion()) {
-        // If the terms should not be compared or if the terms are equal, fall back to version
-        // comparison.
+    } else if (args.getConfigVersionAndTerm() < _rsConfig.getConfigVersionAndTerm()) {
         response->setVoteGranted(false);
         response->setReason(str::stream()
-                            << "ignoring term of -1 for comparison, candidate's version in "
-                               "config(term, version): ({}, {}) is lower than mine ({}, {})"_format(
-                                   args.getConfigTerm(),
-                                   args.getConfigVersion(),
-                                   _rsConfig.getConfigTerm(),
-                                   _rsConfig.getConfigVersion()));
+                            << "candidate's config with {} is older "
+                               "than mine with {}"_format(args.getConfigVersionAndTerm(),
+                                                          _rsConfig.getConfigVersionAndTerm()));
     } else if (args.getSetName() != _rsConfig.getReplSetName()) {
         response->setVoteGranted(false);
         response->setReason(str::stream()
diff --git a/src/mongo/db/repl/topology_coordinator_v1_test.cpp b/src/mongo/db/repl/topology_coordinator_v1_test.cpp
index 6756e50e246..9fc313a6042 100644
--- a/src/mongo/db/repl/topology_coordinator_v1_test.cpp
+++ b/src/mongo/db/repl/topology_coordinator_v1_test.cpp
@@ -2798,8 +2798,8 @@ TEST_F(TopoCoordTest, NodeDoesNotGrantVoteWhenConfigVersionIsLower) {
 
     getTopoCoord().processReplSetRequestVotes(args, &response);
     ASSERT_EQUALS(
-        "ignoring term of -1 for comparison, candidate's version in config(term, version): (1, 0) "
-        "is lower than mine (1, 1)",
+        "candidate's config with {version: 0, term: 1} is older than mine with {version: 1, term: "
+        "1}",
         response.getReason());
     ASSERT_FALSE(response.getVoteGranted());
 }
@@ -2829,8 +2829,10 @@ TEST_F(TopoCoordTest, NodeDoesNotGrantVoteWhenConfigTermIsLower) {
     ReplSetRequestVotesResponse response;
 
     getTopoCoord().processReplSetRequestVotes(args, &response);
-    ASSERT_EQUALS("candidate's term in config(term, version): (1, 1) is lower than mine (2, 1)",
-                  response.getReason());
+    ASSERT_EQUALS(
+        "candidate's config with {version: 1, term: 1} is older than mine with {version: 1, term: "
+        "2}",
+        response.getReason());
     ASSERT_FALSE(response.getVoteGranted());
 }
 
@@ -2997,8 +2999,8 @@ TEST_F(TopoCoordTest, NodeDoesNotGrantDryRunVoteWhenConfigVersionIsLower) {
 
     getTopoCoord().processReplSetRequestVotes(args, &response);
     ASSERT_EQUALS(
-        "ignoring term of -1 for comparison, candidate's version in config(term, version): (1, 0) "
-        "is lower than mine (1, 1)",
+        "candidate's config with {version: 0, term: 1} is older than mine with {version: 1, term: "
+        "1}",
         response.getReason());
     ASSERT_EQUALS(1, response.getTerm());
     ASSERT_FALSE(response.getVoteGranted());