diff options
5 files changed, 112 insertions, 81 deletions
diff --git a/src/mongo/client/sdam/election_id_set_version_pair.h b/src/mongo/client/sdam/election_id_set_version_pair.h index 33e8667c935..296f94b6116 100644 --- a/src/mongo/client/sdam/election_id_set_version_pair.h +++ b/src/mongo/client/sdam/election_id_set_version_pair.h @@ -30,14 +30,14 @@ #include <boost/optional.hpp> +#include "mongo/bson/bsonobjbuilder.h" #include "mongo/bson/oid.h" - namespace mongo::sdam { // Comparable pair or ElectionId (term) and SetVersion. struct ElectionIdSetVersionPair { - const boost::optional<mongo::OID> electionId; - const boost::optional<int> setVersion; + boost::optional<mongo::OID> electionId; + boost::optional<int> setVersion; bool allDefined() const { return electionId && setVersion; @@ -46,6 +46,12 @@ struct ElectionIdSetVersionPair { bool allUndefined() const { return !electionId && !setVersion; } + + bool anyUndefined() const { + return !electionId || !setVersion; + } + + BSONObj toBSON() const; }; inline bool electionIdEqual(const ElectionIdSetVersionPair& p1, @@ -54,19 +60,12 @@ inline bool electionIdEqual(const ElectionIdSetVersionPair& p1, } inline bool operator<(const ElectionIdSetVersionPair& p1, const ElectionIdSetVersionPair& p2) { - if (electionIdEqual(p1, p2)) { - if (p1.setVersion && p2.setVersion) { - return *p1.setVersion < *p2.setVersion; - } - return false; // Cannot compare Set versions. - } - - if (p1.electionId && p2.electionId) { - return (*p1.electionId).compare(*p2.electionId) < 0; + if (p1.anyUndefined() && p2.allDefined()) { + return true; } - // We know that election Ids are not comparable. - return false; + return (p1.electionId < p2.electionId) || + ((p1.electionId == p2.electionId) && (p1.setVersion < p2.setVersion)); } inline bool operator>(const ElectionIdSetVersionPair& p1, const ElectionIdSetVersionPair& p2) { @@ -75,15 +74,7 @@ inline bool operator>(const ElectionIdSetVersionPair& p1, const ElectionIdSetVer // Equality operator is not equivalent to "!< && !>" because of grey area of undefined values. inline bool operator==(const ElectionIdSetVersionPair& p1, const ElectionIdSetVersionPair& p2) { - if (!electionIdEqual(p1, p2)) { - return false; - } - - if (!p1.setVersion && !p2.setVersion) { - return true; - } - - return p1.setVersion && p2.setVersion && *p1.setVersion == *p2.setVersion; + return ((p1.electionId == p2.electionId) && (p1.setVersion == p2.setVersion)); } inline bool setVersionWentBackwards(const ElectionIdSetVersionPair& current, @@ -96,30 +87,26 @@ inline bool setVersionWentBackwards(const ElectionIdSetVersionPair& current, */ inline bool isIncomingPrimaryConsistent(const ElectionIdSetVersionPair& current, const ElectionIdSetVersionPair& incoming) { - // If coming from primary both election Id and Set version should be always set. - // This requirement is true for at least MDB >= 4.0. - if (!incoming.electionId || !incoming.setVersion) { - return false; - } - - if (current.allUndefined()) { - return true; // Further check is not necessary. - } - - // Identical term means Set version should be equal or advance because no failover - // happened and Set version at the same primary cannot go backwards. - if (electionIdEqual(current, incoming)) { - return !current.setVersion || *current.setVersion <= *incoming.setVersion; - } - // If Set version goes backwards the term should advance, because it means // failover happened. This is possible if the previous primary failed to replicate // new Set version before failover. When it happens, we will rollback the Set version. if (setVersionWentBackwards(current, incoming)) { - return !current.electionId || (*current.electionId).compare(*incoming.electionId) < 0; + return !current.electionId || !incoming.electionId || + (*current.electionId).compare(*incoming.electionId) < 0; } return true; } +inline BSONObj ElectionIdSetVersionPair::toBSON() const { + BSONObjBuilder bob; + if (electionId) { + bob.append("electionId", *electionId); + } + if (setVersion) { + bob.append("setVersion", *setVersion); + } + return bob.obj(); +} + } // namespace mongo::sdam diff --git a/src/mongo/client/sdam/election_id_set_version_pair_test.cpp b/src/mongo/client/sdam/election_id_set_version_pair_test.cpp index b375a6760fc..93a95c10097 100644 --- a/src/mongo/client/sdam/election_id_set_version_pair_test.cpp +++ b/src/mongo/client/sdam/election_id_set_version_pair_test.cpp @@ -97,27 +97,27 @@ public: TEST_F(ElectionIdSetVersionPairTest, ExpectedOutcome) { std::vector<TestCase> tests = { - // At startup, both current fields are not set. - {kNullOid, kNullSet, kOidOne, 1, Compare::kNotComparable, Consistency::kConsistent}, + // At startup, both current fields are not set. Field set > unset. + {kNullOid, kNullSet, kOidOne, 1, Compare::kLess, Consistency::kConsistent}, {kOidOne, 1, kOidOne, 1, Compare::kEquals, Consistency::kConsistent}, // One field is not set. This should never happen however added for better // coverage for malformed protocol. - {kNullOid, 1, kOidOne, 1, Compare::kNotComparable, Consistency::kConsistent}, - {kOidOne, kNullSet, kOidOne, 1, Compare::kNotComparable, Consistency::kConsistent}, - {kOidOne, 1, kNullOid, 1, Compare::kNotComparable, Consistency::kInconsistent}, - {kOidOne, 1, kOidOne, kNullSet, Compare::kNotComparable, Consistency::kInconsistent}, + {kNullOid, 1, kOidOne, 1, Compare::kLess, Consistency::kConsistent}, + {kOidOne, kNullSet, kOidOne, 1, Compare::kLess, Consistency::kConsistent}, + {kOidOne, 1, kNullOid, 1, Compare::kGreater, Consistency::kConsistent}, + {kOidOne, 1, kOidOne, kNullSet, Compare::kGreater, Consistency::kConsistent}, // Primary advanced one way or another. "Less" means current < incoming. {kOidOne, 1, kOidTwo, 1, Compare::kLess, Consistency::kConsistent}, {kOidOne, 1, kOidOne, 2, Compare::kLess, Consistency::kConsistent}, // Primary advanced but current state is incomplete. - {kNullOid, 1, kOidTwo, 1, Compare::kNotComparable, Consistency::kConsistent}, - {kNullOid, 1, kOidOne, 2, Compare::kNotComparable, Consistency::kConsistent}, + {kNullOid, 1, kOidTwo, 1, Compare::kLess, Consistency::kConsistent}, + {kNullOid, 1, kOidOne, 2, Compare::kLess, Consistency::kConsistent}, {kOidOne, kNullSet, kOidTwo, 1, Compare::kLess, Consistency::kConsistent}, - {kOidOne, kNullSet, kOidOne, 2, Compare::kNotComparable, Consistency::kConsistent}, + {kOidOne, kNullSet, kOidOne, 2, Compare::kLess, Consistency::kConsistent}, // Primary went backwards one way or another. // Inconsistent because Set version went backwards without Term being changed (same @@ -126,7 +126,7 @@ TEST_F(ElectionIdSetVersionPairTest, ExpectedOutcome) { {kOidTwo, 2, kOidOne, 2, Compare::kGreater, Consistency::kConsistent}, // Primary went backwards with current state incomplete. - {kNullOid, 2, kOidTwo, 1, Compare::kNotComparable, Consistency::kConsistent}, + {kNullOid, 2, kOidTwo, 1, Compare::kLess, Consistency::kConsistent}, {kOidTwo, kNullSet, kOidOne, 1, Compare::kGreater, Consistency::kConsistent}, // Stale primary case when Term went backwards but Set version advanced. @@ -145,5 +145,49 @@ TEST_F(ElectionIdSetVersionPairTest, ExpectedOutcome) { } } +TEST_F(ElectionIdSetVersionPairTest, TestLessThanRelation) { + ElectionIdSetVersionPair hasNone{boost::none, boost::none}; + ElectionIdSetVersionPair hasBoth{kOidOne, 1}; + ElectionIdSetVersionPair hasBoth2{kOidOne, 2}; + ElectionIdSetVersionPair noEid{boost::none, 1}; + ElectionIdSetVersionPair noEid2{boost::none, 2}; + ElectionIdSetVersionPair noSet{kOidOne, boost::none}; + ElectionIdSetVersionPair noSet2{kOidTwo, boost::none}; + + ASSERT_TRUE(hasNone < noEid); + ASSERT(hasNone == hasNone); + + ASSERT_TRUE(hasNone < noEid2); + ASSERT_TRUE(noEid < noEid2); + ASSERT(noEid == noEid); + + ASSERT_TRUE(hasNone < noSet); + ASSERT_TRUE(noEid < noSet); + ASSERT_TRUE(noEid2 < noSet); + ASSERT(noEid2 == noEid2); + + ASSERT_TRUE(hasNone < noSet2); + ASSERT_TRUE(noEid < noSet2); + ASSERT_TRUE(noEid2 < noSet2); + ASSERT_TRUE(noSet < noSet2); + ASSERT(noSet == noSet); + + ASSERT_TRUE(hasNone < hasBoth); + ASSERT_TRUE(noEid < hasBoth); + ASSERT_TRUE(noEid2 < hasBoth); + ASSERT_TRUE(noSet < hasBoth); + ASSERT_TRUE(noSet2 < hasBoth); + ASSERT(noSet2 == noSet2); + + ASSERT_TRUE(hasNone < hasBoth2); + ASSERT_TRUE(noEid < hasBoth2); + ASSERT_TRUE(noEid2 < hasBoth2); + ASSERT_TRUE(noSet < hasBoth2); + ASSERT_TRUE(noSet2 < hasBoth2); + ASSERT_TRUE(hasBoth < hasBoth2); + ASSERT(hasBoth == hasBoth); + ASSERT(hasBoth2 == hasBoth2); +} + } // namespace } // namespace mongo::sdam diff --git a/src/mongo/client/sdam/sdam_json_test_runner.cpp b/src/mongo/client/sdam/sdam_json_test_runner.cpp index 0363d4ca599..749585cf01c 100644 --- a/src/mongo/client/sdam/sdam_json_test_runner.cpp +++ b/src/mongo/client/sdam/sdam_json_test_runner.cpp @@ -223,7 +223,7 @@ private: } return result; }, - serverDescription->getSetVersion()); + serverDescription->getElectionIdSetVersionPair().setVersion); } else if (fieldName == "electionId") { doValidateServerField(result, @@ -236,7 +236,7 @@ private: } return result; }, - serverDescription->getElectionId()); + serverDescription->getElectionIdSetVersionPair().electionId); } else if (fieldName == "logicalSessionTimeoutMinutes") { doValidateServerField(result, @@ -368,36 +368,36 @@ private: { constexpr auto fieldName = "maxSetVersion"; if (bsonTopologyDescription.hasField(fieldName)) { - doValidateTopologyDescriptionField(result, - fieldName, - [&]() { - boost::optional<int> ret; - auto bsonField = - bsonTopologyDescription[fieldName]; - if (!bsonField.isNull()) { - ret = bsonField.numberInt(); - } - return ret; - }, - topologyDescription->getMaxSetVersion()); + doValidateTopologyDescriptionField( + result, + fieldName, + [&]() { + boost::optional<int> ret; + auto bsonField = bsonTopologyDescription[fieldName]; + if (!bsonField.isNull()) { + ret = bsonField.numberInt(); + } + return ret; + }, + topologyDescription->getMaxElectionIdSetVersionPair().setVersion); } } { constexpr auto fieldName = "maxElectionId"; if (bsonTopologyDescription.hasField(fieldName)) { - doValidateTopologyDescriptionField(result, - fieldName, - [&]() { - boost::optional<OID> ret; - auto bsonField = - bsonTopologyDescription[fieldName]; - if (!bsonField.isNull()) { - ret = bsonField.OID(); - } - return ret; - }, - topologyDescription->getMaxElectionId()); + doValidateTopologyDescriptionField( + result, + fieldName, + [&]() { + boost::optional<OID> ret; + auto bsonField = bsonTopologyDescription[fieldName]; + if (!bsonField.isNull()) { + ret = bsonField.OID(); + } + return ret; + }, + topologyDescription->getMaxElectionIdSetVersionPair().electionId); } } diff --git a/src/mongo/client/sdam/topology_description_test.cpp b/src/mongo/client/sdam/topology_description_test.cpp index 9622e3dfdd7..8c741bce134 100644 --- a/src/mongo/client/sdam/topology_description_test.cpp +++ b/src/mongo/client/sdam/topology_description_test.cpp @@ -79,7 +79,7 @@ protected: void TopologyDescriptionTestFixture::assertDefaultConfig( const TopologyDescription& topologyDescription) { ASSERT_EQUALS(boost::none, topologyDescription.getSetName()); - ASSERT_EQUALS(boost::none, topologyDescription.getMaxElectionId()); + ASSERT_EQUALS(boost::none, topologyDescription.getMaxElectionIdSetVersionPair().electionId); auto expectedDefaultServer = ServerDescription(HostAndPort("localhost:27017")); ASSERT_EQUALS(expectedDefaultServer, *topologyDescription.getServers().front()); diff --git a/src/mongo/client/sdam/topology_state_machine_test.cpp b/src/mongo/client/sdam/topology_state_machine_test.cpp index 6671d6eb9e2..5f47b8386f0 100644 --- a/src/mongo/client/sdam/topology_state_machine_test.cpp +++ b/src/mongo/client/sdam/topology_state_machine_test.cpp @@ -378,7 +378,7 @@ TEST_F(TopologyStateMachineTestFixture, ShouldSaveNewMaxSetVersion) { .instance(); stateMachine.onServerDescription(*topologyDescription, serverDescription); - ASSERT_EQUALS(100, topologyDescription->getMaxSetVersion()); + ASSERT_EQUALS(100, topologyDescription->getMaxElectionIdSetVersionPair().setVersion); auto serverDescriptionEvenBiggerSetVersion = ServerDescriptionBuilder() .withType(ServerType::kRSPrimary) @@ -391,7 +391,7 @@ TEST_F(TopologyStateMachineTestFixture, ShouldSaveNewMaxSetVersion) { .instance(); stateMachine.onServerDescription(*topologyDescription, serverDescriptionEvenBiggerSetVersion); - ASSERT_EQUALS(200, topologyDescription->getMaxSetVersion()); + ASSERT_EQUALS(200, topologyDescription->getMaxElectionIdSetVersionPair().setVersion); } TEST_F(TopologyStateMachineTestFixture, ShouldSaveNewMaxElectionId) { @@ -410,7 +410,7 @@ TEST_F(TopologyStateMachineTestFixture, ShouldSaveNewMaxElectionId) { .instance(); stateMachine.onServerDescription(*topologyDescription, serverDescription); - ASSERT_EQUALS(kOidOne, topologyDescription->getMaxElectionId()); + ASSERT_EQUALS(kOidOne, topologyDescription->getMaxElectionIdSetVersionPair().electionId); auto serverDescriptionEvenBiggerElectionId = ServerDescriptionBuilder() .withType(ServerType::kRSPrimary) @@ -423,7 +423,7 @@ TEST_F(TopologyStateMachineTestFixture, ShouldSaveNewMaxElectionId) { .instance(); stateMachine.onServerDescription(*topologyDescription, serverDescriptionEvenBiggerElectionId); - ASSERT_EQUALS(kOidTwo, topologyDescription->getMaxElectionId()); + ASSERT_EQUALS(kOidTwo, topologyDescription->getMaxElectionIdSetVersionPair().electionId); } // The following two tests (ShouldNotUpdateToplogyType, ShouldUpdateToCorrectToplogyType) assert |