From 8c50f98c003b9ec8cbb19f1ef8a93efdefba31c1 Mon Sep 17 00:00:00 2001 From: Lukas Joswiak Date: Tue, 13 Sep 2022 15:43:56 -0700 Subject: [PATCH] Update type of coordinators hash This fixes some serialization issues due to `BinaryReader` not being able to deserialize types of size_t. --- fdbclient/PaxosConfigTransaction.actor.cpp | 13 +++--- fdbclient/include/fdbclient/ConfigKnobs.h | 2 + .../fdbclient/ConfigTransactionInterface.h | 20 ++++----- fdbclient/include/fdbclient/FDBTypes.h | 1 + fdbserver/ConfigBroadcaster.actor.cpp | 4 +- fdbserver/ConfigNode.actor.cpp | 42 +++++++++---------- .../fdbserver/ConfigBroadcastInterface.h | 4 +- .../fdbserver/ConfigFollowerInterface.h | 4 +- 8 files changed, 46 insertions(+), 44 deletions(-) diff --git a/fdbclient/PaxosConfigTransaction.actor.cpp b/fdbclient/PaxosConfigTransaction.actor.cpp index 26a2282236..0918e35062 100644 --- a/fdbclient/PaxosConfigTransaction.actor.cpp +++ b/fdbclient/PaxosConfigTransaction.actor.cpp @@ -35,7 +35,8 @@ class CommitQuorum { Standalone> mutations; ConfigCommitAnnotation annotation; - ConfigTransactionCommitRequest getCommitRequest(ConfigGeneration generation, size_t coordinatorsHash) const { + ConfigTransactionCommitRequest getCommitRequest(ConfigGeneration generation, + CoordinatorsHash coordinatorsHash) const { return ConfigTransactionCommitRequest(coordinatorsHash, generation, mutations, annotation); } @@ -63,7 +64,7 @@ class CommitQuorum { ACTOR static Future addRequestActor(CommitQuorum* self, ConfigGeneration generation, - size_t coordinatorsHash, + CoordinatorsHash coordinatorsHash, ConfigTransactionInterface cti) { try { if (cti.hostname.present()) { @@ -112,7 +113,7 @@ public: } void setTimestamp() { annotation.timestamp = now(); } size_t expectedSize() const { return annotation.expectedSize() + mutations.expectedSize(); } - Future commit(ConfigGeneration generation, size_t coordinatorsHash) { + Future commit(ConfigGeneration generation, CoordinatorsHash coordinatorsHash) { // Send commit message to all replicas, even those that did not return the used replica. // This way, slow replicas are kept up date. for (const auto& cti : ctis) { @@ -125,7 +126,7 @@ public: class GetGenerationQuorum { ActorCollection actors{ false }; - size_t coordinatorsHash{ 0 }; + CoordinatorsHash coordinatorsHash{ 0 }; std::vector ctis; std::map> seenGenerations; Promise result; @@ -241,7 +242,7 @@ class GetGenerationQuorum { public: GetGenerationQuorum() = default; - explicit GetGenerationQuorum(size_t coordinatorsHash, + explicit GetGenerationQuorum(CoordinatorsHash coordinatorsHash, std::vector const& ctis, Future coordinatorsChangedFuture, Optional const& lastSeenLiveVersion = {}) @@ -267,7 +268,7 @@ public: }; class PaxosConfigTransactionImpl { - size_t coordinatorsHash{ 0 }; + CoordinatorsHash coordinatorsHash{ 0 }; std::vector ctis; GetGenerationQuorum getGenerationQuorum; CommitQuorum commitQuorum; diff --git a/fdbclient/include/fdbclient/ConfigKnobs.h b/fdbclient/include/fdbclient/ConfigKnobs.h index 536bca16f3..168e2fed16 100644 --- a/fdbclient/include/fdbclient/ConfigKnobs.h +++ b/fdbclient/include/fdbclient/ConfigKnobs.h @@ -25,6 +25,8 @@ #include "fdbclient/FDBTypes.h" +typedef uint64_t CoordinatorsHash; + /* * KnobValueRefs are stored in the configuration database, and in local configuration files. They are created from * ParsedKnobValue objects, so it is assumed that the value type is correct for the corresponding knob name diff --git a/fdbclient/include/fdbclient/ConfigTransactionInterface.h b/fdbclient/include/fdbclient/ConfigTransactionInterface.h index fd0cd5922d..dad60f2d04 100644 --- a/fdbclient/include/fdbclient/ConfigTransactionInterface.h +++ b/fdbclient/include/fdbclient/ConfigTransactionInterface.h @@ -65,12 +65,12 @@ struct ConfigTransactionGetGenerationReply { struct ConfigTransactionGetGenerationRequest { static constexpr FileIdentifier file_identifier = 138941; - size_t coordinatorsHash{ 0 }; + CoordinatorsHash coordinatorsHash{ 0 }; // A hint to catch up lagging nodes: Optional lastSeenLiveVersion; ReplyPromise reply; ConfigTransactionGetGenerationRequest() = default; - explicit ConfigTransactionGetGenerationRequest(size_t coordinatorsHash, + explicit ConfigTransactionGetGenerationRequest(CoordinatorsHash coordinatorsHash, Optional const& lastSeenLiveVersion) : coordinatorsHash(coordinatorsHash), lastSeenLiveVersion(lastSeenLiveVersion) {} @@ -94,13 +94,13 @@ struct ConfigTransactionGetReply { struct ConfigTransactionGetRequest { static constexpr FileIdentifier file_identifier = 923040; - size_t coordinatorsHash{ 0 }; + CoordinatorsHash coordinatorsHash{ 0 }; ConfigGeneration generation; ConfigKey key; ReplyPromise reply; ConfigTransactionGetRequest() = default; - explicit ConfigTransactionGetRequest(size_t coordinatorsHash, ConfigGeneration generation, ConfigKey key) + explicit ConfigTransactionGetRequest(CoordinatorsHash coordinatorsHash, ConfigGeneration generation, ConfigKey key) : coordinatorsHash(coordinatorsHash), generation(generation), key(key) {} template @@ -112,14 +112,14 @@ struct ConfigTransactionGetRequest { struct ConfigTransactionCommitRequest { static constexpr FileIdentifier file_identifier = 103841; Arena arena; - size_t coordinatorsHash{ 0 }; + CoordinatorsHash coordinatorsHash{ 0 }; ConfigGeneration generation{ ::invalidVersion, ::invalidVersion }; VectorRef mutations; ConfigCommitAnnotationRef annotation; ReplyPromise reply; ConfigTransactionCommitRequest() = default; - explicit ConfigTransactionCommitRequest(size_t coordinatorsHash, + explicit ConfigTransactionCommitRequest(CoordinatorsHash coordinatorsHash, ConfigGeneration generation, VectorRef mutations, ConfigCommitAnnotationRef annotation) @@ -150,12 +150,12 @@ struct ConfigTransactionGetConfigClassesReply { struct ConfigTransactionGetConfigClassesRequest { static constexpr FileIdentifier file_identifier = 7163400; - size_t coordinatorsHash{ 0 }; + CoordinatorsHash coordinatorsHash{ 0 }; ConfigGeneration generation; ReplyPromise reply; ConfigTransactionGetConfigClassesRequest() = default; - explicit ConfigTransactionGetConfigClassesRequest(size_t coordinatorsHash, ConfigGeneration generation) + explicit ConfigTransactionGetConfigClassesRequest(CoordinatorsHash coordinatorsHash, ConfigGeneration generation) : coordinatorsHash(coordinatorsHash), generation(generation) {} template @@ -179,13 +179,13 @@ struct ConfigTransactionGetKnobsReply { struct ConfigTransactionGetKnobsRequest { static constexpr FileIdentifier file_identifier = 987410; - size_t coordinatorsHash{ 0 }; + CoordinatorsHash coordinatorsHash{ 0 }; ConfigGeneration generation; Optional configClass; ReplyPromise reply; ConfigTransactionGetKnobsRequest() = default; - explicit ConfigTransactionGetKnobsRequest(size_t coordinatorsHash, + explicit ConfigTransactionGetKnobsRequest(CoordinatorsHash coordinatorsHash, ConfigGeneration generation, Optional configClass) : coordinatorsHash(coordinatorsHash), generation(generation), configClass(configClass) {} diff --git a/fdbclient/include/fdbclient/FDBTypes.h b/fdbclient/include/fdbclient/FDBTypes.h index 7c4f82b4ce..fdd788a474 100644 --- a/fdbclient/include/fdbclient/FDBTypes.h +++ b/fdbclient/include/fdbclient/FDBTypes.h @@ -41,6 +41,7 @@ typedef StringRef KeyRef; typedef StringRef ValueRef; typedef int64_t Generation; typedef UID SpanID; +typedef uint64_t CoordinatorsHash; enum { tagLocalitySpecial = -1, // tag with this locality means it is invalidTag (id=0), txsTag (id=1), or cacheTag (id=2) diff --git a/fdbserver/ConfigBroadcaster.actor.cpp b/fdbserver/ConfigBroadcaster.actor.cpp index f417fd2dfb..eafce9c9cb 100644 --- a/fdbserver/ConfigBroadcaster.actor.cpp +++ b/fdbserver/ConfigBroadcaster.actor.cpp @@ -80,7 +80,7 @@ class ConfigBroadcasterImpl { Version lastCompactedVersion; Version largestLiveVersion; Version mostRecentVersion; - size_t coordinatorsHash; + CoordinatorsHash coordinatorsHash; std::unique_ptr consumer; Future consumerFuture; ActorCollection actors{ false }; @@ -717,7 +717,7 @@ ACTOR static Future lockConfigNodesImpl(ServerCoordinators coordinators) { return Void(); } - size_t coordinatorsHash = std::hash()(coordinators.ccr->getConnectionString().toString()); + CoordinatorsHash coordinatorsHash = std::hash()(coordinators.ccr->getConnectionString().toString()); std::vector> lockRequests; lockRequests.reserve(coordinators.configServers.size()); diff --git a/fdbserver/ConfigNode.actor.cpp b/fdbserver/ConfigNode.actor.cpp index 6c2ce00944..fce3ee5010 100644 --- a/fdbserver/ConfigNode.actor.cpp +++ b/fdbserver/ConfigNode.actor.cpp @@ -135,11 +135,11 @@ class ConfigNodeImpl { Counter getGenerationRequests; Future logger; - ACTOR static Future getCoordinatorsHash(ConfigNodeImpl* self) { - state size_t coordinatorsHash = 0; + ACTOR static Future getCoordinatorsHash(ConfigNodeImpl* self) { + state CoordinatorsHash coordinatorsHash = 0; Optional value = wait(self->kvStore->readValue(coordinatorsHashKey)); if (value.present()) { - coordinatorsHash = BinaryReader::fromStringRef(value.get(), IncludeVersion()); + coordinatorsHash = BinaryReader::fromStringRef(value.get(), IncludeVersion()); } else { self->kvStore->set( KeyValueRef(coordinatorsHashKey, BinaryWriter::toValue(coordinatorsHash, IncludeVersion()))); @@ -148,14 +148,12 @@ class ConfigNodeImpl { return coordinatorsHash; } - // The returned value is a hash of the nodes current idea of the - // coordinators. - ACTOR static Future> getLocked(ConfigNodeImpl* self) { + ACTOR static Future> getLocked(ConfigNodeImpl* self) { Optional value = wait(self->kvStore->readValue(lockedKey)); if (!value.present()) { - return Optional(); + return Optional(); } - return BinaryReader::fromStringRef>(value.get(), IncludeVersion()); + return BinaryReader::fromStringRef>(value.get(), IncludeVersion()); } ACTOR static Future getGeneration(ConfigNodeImpl* self) { @@ -254,7 +252,7 @@ class ConfigNodeImpl { // New transactions increment the database's current live version. This effectively serves as a lock, providing // serializability ACTOR static Future getNewGeneration(ConfigNodeImpl* self, ConfigTransactionGetGenerationRequest req) { - state size_t coordinatorsHash = wait(getCoordinatorsHash(self)); + state CoordinatorsHash coordinatorsHash = wait(getCoordinatorsHash(self)); if (req.coordinatorsHash != coordinatorsHash) { req.reply.sendError(coordinators_changed()); return Void(); @@ -273,13 +271,13 @@ class ConfigNodeImpl { } ACTOR static Future get(ConfigNodeImpl* self, ConfigTransactionGetRequest req) { - state Optional locked = wait(getLocked(self)); + state Optional locked = wait(getLocked(self)); if (locked.present()) { CODE_PROBE(true, "attempting to read from a locked ConfigNode"); req.reply.sendError(coordinators_changed()); return Void(); } - state size_t coordinatorsHash = wait(getCoordinatorsHash(self)); + state CoordinatorsHash coordinatorsHash = wait(getCoordinatorsHash(self)); if (req.coordinatorsHash != coordinatorsHash) { req.reply.sendError(coordinators_changed()); return Void(); @@ -316,13 +314,13 @@ class ConfigNodeImpl { // TODO: Currently it is possible that extra configuration classes may be returned, we // may want to fix this to clean up the contract ACTOR static Future getConfigClasses(ConfigNodeImpl* self, ConfigTransactionGetConfigClassesRequest req) { - state Optional locked = wait(getLocked(self)); + state Optional locked = wait(getLocked(self)); if (locked.present()) { CODE_PROBE(true, "attempting to read config classes from locked ConfigNode"); req.reply.sendError(coordinators_changed()); return Void(); } - state size_t coordinatorsHash = wait(getCoordinatorsHash(self)); + state CoordinatorsHash coordinatorsHash = wait(getCoordinatorsHash(self)); if (req.coordinatorsHash != coordinatorsHash) { req.reply.sendError(coordinators_changed()); return Void(); @@ -360,13 +358,13 @@ class ConfigNodeImpl { // Retrieve all knobs explicitly defined for the specified configuration class ACTOR static Future getKnobs(ConfigNodeImpl* self, ConfigTransactionGetKnobsRequest req) { - state Optional locked = wait(getLocked(self)); + state Optional locked = wait(getLocked(self)); if (locked.present()) { CODE_PROBE(true, "attempting to read knobs from locked ConfigNode"); req.reply.sendError(coordinators_changed()); return Void(); } - state size_t coordinatorsHash = wait(getCoordinatorsHash(self)); + state CoordinatorsHash coordinatorsHash = wait(getCoordinatorsHash(self)); if (req.coordinatorsHash != coordinatorsHash) { req.reply.sendError(coordinators_changed()); return Void(); @@ -453,13 +451,13 @@ class ConfigNodeImpl { } ACTOR static Future commit(ConfigNodeImpl* self, ConfigTransactionCommitRequest req) { - state Optional locked = wait(getLocked(self)); + state Optional locked = wait(getLocked(self)); if (locked.present()) { CODE_PROBE(true, "attempting to write to locked ConfigNode"); req.reply.sendError(coordinators_changed()); return Void(); } - state size_t coordinatorsHash = wait(getCoordinatorsHash(self)); + state CoordinatorsHash coordinatorsHash = wait(getCoordinatorsHash(self)); if (req.coordinatorsHash != coordinatorsHash) { req.reply.sendError(coordinators_changed()); return Void(); @@ -675,7 +673,7 @@ class ConfigNodeImpl { req.reply.send(ConfigBroadcastRegisteredReply{ isRegistered, generation.committedVersion }); } when(state ConfigBroadcastReadyRequest readyReq = waitNext(cbi->ready.getFuture())) { - state Optional locked = wait(getLocked(self)); + state Optional locked = wait(getLocked(self)); // New ConfigNodes with no previous state should always // apply snapshots from the ConfigBroadcaster. Otherwise, @@ -715,8 +713,8 @@ class ConfigNodeImpl { // Make sure freshly up to date ConfigNode isn't // locked! This is possible if it was a coordinator in // a previous generation. - self->kvStore->set( - KeyValueRef(lockedKey, BinaryWriter::toValue(Optional(), IncludeVersion()))); + self->kvStore->set(KeyValueRef( + lockedKey, BinaryWriter::toValue(Optional(), IncludeVersion()))); } self->kvStore->set(KeyValueRef(coordinatorsHashKey, BinaryWriter::toValue(readyReq.coordinatorsHash, IncludeVersion()))); @@ -768,13 +766,13 @@ class ConfigNodeImpl { } when(state ConfigFollowerLockRequest req = waitNext(cfi->lock.getFuture())) { ++self->lockRequests; - size_t coordinatorsHash = wait(getCoordinatorsHash(self)); + CoordinatorsHash coordinatorsHash = wait(getCoordinatorsHash(self)); if (coordinatorsHash == 0 || coordinatorsHash == req.coordinatorsHash) { TraceEvent("ConfigNodeLocking", self->id).log(); self->kvStore->set(KeyValueRef(registeredKey, BinaryWriter::toValue(false, IncludeVersion()))); self->kvStore->set(KeyValueRef( lockedKey, - BinaryWriter::toValue(Optional(req.coordinatorsHash), IncludeVersion()))); + BinaryWriter::toValue(Optional(req.coordinatorsHash), IncludeVersion()))); wait(self->kvStore->commit()); } req.reply.send(Void()); diff --git a/fdbserver/include/fdbserver/ConfigBroadcastInterface.h b/fdbserver/include/fdbserver/ConfigBroadcastInterface.h index 2f15f490bf..1c7733680b 100644 --- a/fdbserver/include/fdbserver/ConfigBroadcastInterface.h +++ b/fdbserver/include/fdbserver/ConfigBroadcastInterface.h @@ -156,14 +156,14 @@ struct ConfigBroadcastReadyReply { struct ConfigBroadcastReadyRequest { static constexpr FileIdentifier file_identifier = 7402862; - size_t coordinatorsHash{ 0 }; + CoordinatorsHash coordinatorsHash{ 0 }; std::map snapshot; Version snapshotVersion{ 0 }; Version liveVersion{ 0 }; ReplyPromise reply; ConfigBroadcastReadyRequest() = default; - ConfigBroadcastReadyRequest(size_t coordinatorsHash, + ConfigBroadcastReadyRequest(CoordinatorsHash coordinatorsHash, std::map const& snapshot, Version snapshotVersion, Version liveVersion) diff --git a/fdbserver/include/fdbserver/ConfigFollowerInterface.h b/fdbserver/include/fdbserver/ConfigFollowerInterface.h index 46e55d8dd6..aef438255c 100644 --- a/fdbserver/include/fdbserver/ConfigFollowerInterface.h +++ b/fdbserver/include/fdbserver/ConfigFollowerInterface.h @@ -211,11 +211,11 @@ struct ConfigFollowerGetCommittedVersionRequest { struct ConfigFollowerLockRequest { static constexpr FileIdentifier file_identifier = 1867800; - size_t coordinatorsHash{ 0 }; + CoordinatorsHash coordinatorsHash{ 0 }; ReplyPromise reply; ConfigFollowerLockRequest() = default; - explicit ConfigFollowerLockRequest(size_t coordinatorsHash) : coordinatorsHash(coordinatorsHash) {} + explicit ConfigFollowerLockRequest(CoordinatorsHash coordinatorsHash) : coordinatorsHash(coordinatorsHash) {} template void serialize(Ar& ar) {