From d599c7bcbd2aa9299d444bec2b75f795238a3a88 Mon Sep 17 00:00:00 2001 From: Zhe Wu Date: Thu, 31 Aug 2023 11:06:38 -0700 Subject: [PATCH 1/9] Use regex to validate wiggle locality --- fdbclient/FDBTypes.cpp | 12 ++++++++++++ fdbclient/include/fdbclient/FDBTypes.h | 9 ++++++--- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/fdbclient/FDBTypes.cpp b/fdbclient/FDBTypes.cpp index 64838ca4b6..211a509abf 100644 --- a/fdbclient/FDBTypes.cpp +++ b/fdbclient/FDBTypes.cpp @@ -251,4 +251,16 @@ KeyValueStoreType KeyValueStoreType::fromString(const std::string& str) { throw unknown_storage_engine(); } return it->second; +} + + +TEST_CASE("/PerpetualStorageWiggleLocality/Validation") { + ASSERT(isValidPerpetualStorageWiggleLocality("aaa:bbb")); + ASSERT(isValidPerpetualStorageWiggleLocality("aaa:bbb;ccc:ddd")); + ASSERT(isValidPerpetualStorageWiggleLocality("0")); + + ASSERT(!isValidPerpetualStorageWiggleLocality("aaa:bbb;")); + ASSERT(!isValidPerpetualStorageWiggleLocality("aaa:bbb;ccc")); + + return Void(); } \ No newline at end of file diff --git a/fdbclient/include/fdbclient/FDBTypes.h b/fdbclient/include/fdbclient/FDBTypes.h index 2aa8084faa..99504b2e98 100644 --- a/fdbclient/include/fdbclient/FDBTypes.h +++ b/fdbclient/include/fdbclient/FDBTypes.h @@ -24,6 +24,7 @@ #include #include #include +#include #include #include #include @@ -1612,10 +1613,12 @@ struct DatabaseSharedState { : protocolVersion(currentProtocolVersion()), mutexLock(Mutex()), grvCacheSpace(GRVCacheSpace()), refCount(0) {} }; +const static std::regex wiggleLocalityValidation("\\w+:\\w+(;\\w:\\w)*"); inline bool isValidPerpetualStorageWiggleLocality(std::string locality) { - int pos = locality.find(':'); - // locality should be either 0 or in the format ':' - return ((pos > 0 && pos < locality.size() - 1) || locality == "0"); + if (locality == "0") { + return true; + } + return std::regex_match(locality, wiggleLocalityValidation); } // matches what's in fdb_c.h From 4abd2edcad98eaf1893a1265f9d8b21babde1002 Mon Sep 17 00:00:00 2001 From: Zhe Wu Date: Thu, 31 Aug 2023 13:32:06 -0700 Subject: [PATCH 2/9] Parse wiggle locality as a list --- fdbclient/include/fdbclient/FDBTypes.h | 2 +- fdbserver/DDTeamCollection.actor.cpp | 77 ++++++++++++++++++++------ tests/SpecificUnitTest.txt | 3 +- 3 files changed, 62 insertions(+), 20 deletions(-) diff --git a/fdbclient/include/fdbclient/FDBTypes.h b/fdbclient/include/fdbclient/FDBTypes.h index 99504b2e98..331982623f 100644 --- a/fdbclient/include/fdbclient/FDBTypes.h +++ b/fdbclient/include/fdbclient/FDBTypes.h @@ -1613,7 +1613,7 @@ struct DatabaseSharedState { : protocolVersion(currentProtocolVersion()), mutexLock(Mutex()), grvCacheSpace(GRVCacheSpace()), refCount(0) {} }; -const static std::regex wiggleLocalityValidation("\\w+:\\w+(;\\w:\\w)*"); +const static std::regex wiggleLocalityValidation("(\\w+:\\w+)(;\\w+:\\w+)*"); inline bool isValidPerpetualStorageWiggleLocality(std::string locality) { if (locality == "0") { return true; diff --git a/fdbserver/DDTeamCollection.actor.cpp b/fdbserver/DDTeamCollection.actor.cpp index a4f7b447e3..8b4ac44f04 100644 --- a/fdbserver/DDTeamCollection.actor.cpp +++ b/fdbserver/DDTeamCollection.actor.cpp @@ -27,6 +27,7 @@ #include "flow/actorcompiler.h" // This must be the last #include. #include "flow/network.h" #include +#include namespace { @@ -38,17 +39,57 @@ auto get(MapContainer& m, K const& k) -> decltype(m.at(k)) { return it->second; } -void ParsePerpetualStorageWiggleLocality(const std::string& localityKeyValue, - Optional* localityKey, - Optional* localityValue) { +std::vector, Optional>> ParsePerpetualStorageWiggleLocality(const std::string& localityKeyValues) { // parsing format is like "datahall:0" - ASSERT(isValidPerpetualStorageWiggleLocality(localityKeyValue)); + ASSERT(isValidPerpetualStorageWiggleLocality(localityKeyValues)); - // get key and value from perpetual_storage_wiggle_locality. - int split = localityKeyValue.find(':'); - *localityKey = Optional(ValueRef((uint8_t*)localityKeyValue.c_str(), split)); - *localityValue = - Optional(ValueRef((uint8_t*)localityKeyValue.c_str() + split + 1, localityKeyValue.size() - split - 1)); + std::vector, Optional>> parsedLocalities; + + std::vector splitLocalityKeyValues; + boost::split(splitLocalityKeyValues, localityKeyValues, [](char c){ return c == ';';}); + + for (const auto& localityKeyValue : splitLocalityKeyValues) { + ASSERT(!localityKeyValue.empty()); + + // get key and value from perpetual_storage_wiggle_locality. + int split = localityKeyValue.find(':'); + auto key = Optional(ValueRef((uint8_t*)localityKeyValue.c_str(), split)); + auto value = + Optional(ValueRef((uint8_t*)localityKeyValue.c_str() + split + 1, localityKeyValue.size() - split - 1)); + parsedLocalities.push_back(std::make_pair(key, value)); + } + + return parsedLocalities; +} + +TEST_CASE("/DataDistribution/StorageWiggler/ParsePerpetualStorageWiggleLocality") { + { + auto localityKeyValues = ParsePerpetualStorageWiggleLocality("aaa:bbb"); + ASSERT(localityKeyValues.size() == 1); + ASSERT(localityKeyValues[0].first.get() == "aaa"); + ASSERT(localityKeyValues[0].second.get() == "bbb"); + } + + { + auto localityKeyValues = ParsePerpetualStorageWiggleLocality("aaa:bbb;ccc:ddd"); + ASSERT(localityKeyValues.size() == 2); + ASSERT(localityKeyValues[0].first.get() == "aaa"); + ASSERT(localityKeyValues[0].second.get() == "bbb"); + ASSERT(localityKeyValues[1].first.get() == "ccc"); + ASSERT(localityKeyValues[1].second.get() == "ddd"); + } + + { + auto localityKeyValues = ParsePerpetualStorageWiggleLocality("aaa:111;bbb:222;ccc:3dd"); + ASSERT(localityKeyValues.size() == 3); + ASSERT(localityKeyValues[0].first.get() == "aaa"); + ASSERT(localityKeyValues[0].second.get() == "111"); + ASSERT(localityKeyValues[1].first.get() == "bbb"); + ASSERT(localityKeyValues[1].second.get() == "222"); + ASSERT(localityKeyValues[2].first.get() == "ccc"); + ASSERT(localityKeyValues[2].second.get() == "3dd"); + } + return Void(); } } // namespace @@ -2417,11 +2458,10 @@ public: if (self->configuration.perpetualStorageWiggleLocality == "0") { isr.storeType = self->configuration.perpetualStoreType; } else { - Optional localityKey; - Optional localityValue; - ParsePerpetualStorageWiggleLocality( - self->configuration.perpetualStorageWiggleLocality, &localityKey, &localityValue); - if (candidateWorker.worker.locality.get(localityKey.get()) == localityValue) { + std::vector, Optional>> localityKeyValues = ParsePerpetualStorageWiggleLocality( + self->configuration.perpetualStorageWiggleLocality); + ASSERT(localityKeyValues.size() == 1); + if (candidateWorker.worker.locality.get(localityKeyValues[0].first.get()) == localityKeyValues[0].second) { isr.storeType = self->configuration.perpetualStoreType; } } @@ -3951,16 +3991,17 @@ Future DDTeamCollection::monitorHealthyTeams() { } Future DDTeamCollection::getNextWigglingServerID() { - Optional localityKey; - Optional localityValue; + std::vector, Optional>> localityFilter; // NOTE: because normal \xff/conf change through `changeConfig` now will cause DD throw `movekeys_conflict()` // then recruit a new DD, we only need to read current configuration once if (configuration.perpetualStorageWiggleLocality != "0") { - ParsePerpetualStorageWiggleLocality(configuration.perpetualStorageWiggleLocality, &localityKey, &localityValue); + localityFilter = ParsePerpetualStorageWiggleLocality(configuration.perpetualStorageWiggleLocality); + ASSERT(localityFilter.size() == 1); + return DDTeamCollectionImpl::getNextWigglingServerID(storageWiggler, localityFilter[0].first, localityFilter[0].second, this); } - return DDTeamCollectionImpl::getNextWigglingServerID(storageWiggler, localityKey, localityValue, this); + return DDTeamCollectionImpl::getNextWigglingServerID(storageWiggler, Optional(), Optional(), this); } Future DDTeamCollection::readStorageWiggleMap() { diff --git a/tests/SpecificUnitTest.txt b/tests/SpecificUnitTest.txt index 0a8e2e2eab..f3405d79d5 100644 --- a/tests/SpecificUnitTest.txt +++ b/tests/SpecificUnitTest.txt @@ -5,4 +5,5 @@ runSetup=false testName=UnitTests maxTestCases=0 - testsMatching=/status/json/builder + testsMatching=/DataDistribution/StorageWiggler/ParsePerpetualStorageWiggleLocality + # testsMatching=/PerpetualStorageWiggleLocality/Validation From 7158d702c6029ed7fc8a9e9c53d69a346ba3d153 Mon Sep 17 00:00:00 2001 From: Zhe Wu Date: Thu, 31 Aug 2023 13:44:58 -0700 Subject: [PATCH 3/9] Use locality list for host checking --- fdbserver/DDTeamCollection.actor.cpp | 32 ++++++++++++++-------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/fdbserver/DDTeamCollection.actor.cpp b/fdbserver/DDTeamCollection.actor.cpp index 8b4ac44f04..bbf531ae64 100644 --- a/fdbserver/DDTeamCollection.actor.cpp +++ b/fdbserver/DDTeamCollection.actor.cpp @@ -2460,9 +2460,11 @@ public: } else { std::vector, Optional>> localityKeyValues = ParsePerpetualStorageWiggleLocality( self->configuration.perpetualStorageWiggleLocality); - ASSERT(localityKeyValues.size() == 1); - if (candidateWorker.worker.locality.get(localityKeyValues[0].first.get()) == localityKeyValues[0].second) { - isr.storeType = self->configuration.perpetualStoreType; + for (const auto& [localityKey, localityValue] : localityKeyValues) { + if (candidateWorker.worker.locality.get(localityKey.get()) == localityValue) { + isr.storeType = self->configuration.perpetualStoreType; + break; + } } } } @@ -3010,8 +3012,7 @@ public: } ACTOR static Future getNextWigglingServerID(Reference wiggler, - Optional localityKey = Optional(), - Optional localityValue = Optional(), + std::vector, Optional>> localityKeyValues = std::vector, Optional>>(), DDTeamCollection* teamCollection = nullptr) { ASSERT(wiggler->teamCollection == teamCollection); loop { @@ -3024,13 +3025,15 @@ public: } // if perpetual_storage_wiggle_locality has value and not 0(disabled). - if (localityKey.present()) { + if (!localityKeyValues.empty()) { // Whether the selected server matches the locality auto server = teamCollection->server_info.at(id.get()); - // TraceEvent("PerpetualLocality").detail("Server", server->getLastKnownInterface().locality.get(localityKey)).detail("Desire", localityValue); - if (server->getLastKnownInterface().locality.get(localityKey.get()) == localityValue) { - return id.get(); + for (const auto& [localityKey, localityValue] : localityKeyValues){ + // TraceEvent("PerpetualLocality").detail("Server", server->getLastKnownInterface().locality.get(localityKey)).detail("Desire", localityValue); + if (server->getLastKnownInterface().locality.get(localityKey.get()) == localityValue) { + return id.get(); + } } if (wiggler->empty()) { @@ -3991,17 +3994,14 @@ Future DDTeamCollection::monitorHealthyTeams() { } Future DDTeamCollection::getNextWigglingServerID() { - std::vector, Optional>> localityFilter; + std::vector, Optional>> localityKeyValues; // NOTE: because normal \xff/conf change through `changeConfig` now will cause DD throw `movekeys_conflict()` // then recruit a new DD, we only need to read current configuration once if (configuration.perpetualStorageWiggleLocality != "0") { - localityFilter = ParsePerpetualStorageWiggleLocality(configuration.perpetualStorageWiggleLocality); - ASSERT(localityFilter.size() == 1); - return DDTeamCollectionImpl::getNextWigglingServerID(storageWiggler, localityFilter[0].first, localityFilter[0].second, this); + localityKeyValues = ParsePerpetualStorageWiggleLocality(configuration.perpetualStorageWiggleLocality); } - - return DDTeamCollectionImpl::getNextWigglingServerID(storageWiggler, Optional(), Optional(), this); + return DDTeamCollectionImpl::getNextWigglingServerID(storageWiggler, localityKeyValues, this); } Future DDTeamCollection::readStorageWiggleMap() { @@ -6777,7 +6777,7 @@ TEST_CASE("/DataDistribution/StorageWiggler/NextIdWithTSS") { ASSERT(!wiggler->getNextServerId(true).present()); ASSERT(wiggler->getNextServerId(collection->reachTSSPairTarget()) == UID(1, 0)); UID id = wait( - DDTeamCollectionImpl::getNextWigglingServerID(wiggler, Optional(), Optional(), collection.get())); + DDTeamCollectionImpl::getNextWigglingServerID(wiggler, {}, collection.get())); ASSERT(now() - startTime < SERVER_KNOBS->DD_STORAGE_WIGGLE_MIN_SS_AGE_SEC + 150.0); ASSERT(id == UID(2, 0)); return Void(); From a86a2d752e80e17665172e594733888288aa8f92 Mon Sep 17 00:00:00 2001 From: Zhe Wu Date: Thu, 31 Aug 2023 13:46:35 -0700 Subject: [PATCH 4/9] Apply format --- fdbclient/FDBTypes.cpp | 1 - fdbserver/DDTeamCollection.actor.cpp | 28 +++++++++++++++------------- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/fdbclient/FDBTypes.cpp b/fdbclient/FDBTypes.cpp index 211a509abf..75fee4bbf3 100644 --- a/fdbclient/FDBTypes.cpp +++ b/fdbclient/FDBTypes.cpp @@ -253,7 +253,6 @@ KeyValueStoreType KeyValueStoreType::fromString(const std::string& str) { return it->second; } - TEST_CASE("/PerpetualStorageWiggleLocality/Validation") { ASSERT(isValidPerpetualStorageWiggleLocality("aaa:bbb")); ASSERT(isValidPerpetualStorageWiggleLocality("aaa:bbb;ccc:ddd")); diff --git a/fdbserver/DDTeamCollection.actor.cpp b/fdbserver/DDTeamCollection.actor.cpp index bbf531ae64..d18836e075 100644 --- a/fdbserver/DDTeamCollection.actor.cpp +++ b/fdbserver/DDTeamCollection.actor.cpp @@ -39,14 +39,15 @@ auto get(MapContainer& m, K const& k) -> decltype(m.at(k)) { return it->second; } -std::vector, Optional>> ParsePerpetualStorageWiggleLocality(const std::string& localityKeyValues) { +std::vector, Optional>> ParsePerpetualStorageWiggleLocality( + const std::string& localityKeyValues) { // parsing format is like "datahall:0" ASSERT(isValidPerpetualStorageWiggleLocality(localityKeyValues)); std::vector, Optional>> parsedLocalities; std::vector splitLocalityKeyValues; - boost::split(splitLocalityKeyValues, localityKeyValues, [](char c){ return c == ';';}); + boost::split(splitLocalityKeyValues, localityKeyValues, [](char c) { return c == ';'; }); for (const auto& localityKeyValue : splitLocalityKeyValues) { ASSERT(!localityKeyValue.empty()); @@ -54,8 +55,8 @@ std::vector, Optional>> ParsePerpetualStorageWi // get key and value from perpetual_storage_wiggle_locality. int split = localityKeyValue.find(':'); auto key = Optional(ValueRef((uint8_t*)localityKeyValue.c_str(), split)); - auto value = - Optional(ValueRef((uint8_t*)localityKeyValue.c_str() + split + 1, localityKeyValue.size() - split - 1)); + auto value = Optional( + ValueRef((uint8_t*)localityKeyValue.c_str() + split + 1, localityKeyValue.size() - split - 1)); parsedLocalities.push_back(std::make_pair(key, value)); } @@ -78,7 +79,7 @@ TEST_CASE("/DataDistribution/StorageWiggler/ParsePerpetualStorageWiggleLocality" ASSERT(localityKeyValues[1].first.get() == "ccc"); ASSERT(localityKeyValues[1].second.get() == "ddd"); } - + { auto localityKeyValues = ParsePerpetualStorageWiggleLocality("aaa:111;bbb:222;ccc:3dd"); ASSERT(localityKeyValues.size() == 3); @@ -2458,8 +2459,8 @@ public: if (self->configuration.perpetualStorageWiggleLocality == "0") { isr.storeType = self->configuration.perpetualStoreType; } else { - std::vector, Optional>> localityKeyValues = ParsePerpetualStorageWiggleLocality( - self->configuration.perpetualStorageWiggleLocality); + std::vector, Optional>> localityKeyValues = + ParsePerpetualStorageWiggleLocality(self->configuration.perpetualStorageWiggleLocality); for (const auto& [localityKey, localityValue] : localityKeyValues) { if (candidateWorker.worker.locality.get(localityKey.get()) == localityValue) { isr.storeType = self->configuration.perpetualStoreType; @@ -3011,9 +3012,11 @@ public: } } - ACTOR static Future getNextWigglingServerID(Reference wiggler, - std::vector, Optional>> localityKeyValues = std::vector, Optional>>(), - DDTeamCollection* teamCollection = nullptr) { + ACTOR static Future getNextWigglingServerID( + Reference wiggler, + std::vector, Optional>> localityKeyValues = + std::vector, Optional>>(), + DDTeamCollection* teamCollection = nullptr) { ASSERT(wiggler->teamCollection == teamCollection); loop { // when the DC need more @@ -3029,7 +3032,7 @@ public: // Whether the selected server matches the locality auto server = teamCollection->server_info.at(id.get()); - for (const auto& [localityKey, localityValue] : localityKeyValues){ + for (const auto& [localityKey, localityValue] : localityKeyValues) { // TraceEvent("PerpetualLocality").detail("Server", server->getLastKnownInterface().locality.get(localityKey)).detail("Desire", localityValue); if (server->getLastKnownInterface().locality.get(localityKey.get()) == localityValue) { return id.get(); @@ -6776,8 +6779,7 @@ TEST_CASE("/DataDistribution/StorageWiggler/NextIdWithTSS") { KeyValueStoreType::SSD_BTREE_V2)); ASSERT(!wiggler->getNextServerId(true).present()); ASSERT(wiggler->getNextServerId(collection->reachTSSPairTarget()) == UID(1, 0)); - UID id = wait( - DDTeamCollectionImpl::getNextWigglingServerID(wiggler, {}, collection.get())); + UID id = wait(DDTeamCollectionImpl::getNextWigglingServerID(wiggler, {}, collection.get())); ASSERT(now() - startTime < SERVER_KNOBS->DD_STORAGE_WIGGLE_MIN_SS_AGE_SEC + 150.0); ASSERT(id == UID(2, 0)); return Void(); From af42816b0e9b191748731868c116b15232fe8361 Mon Sep 17 00:00:00 2001 From: Zhe Wu Date: Thu, 31 Aug 2023 14:36:36 -0700 Subject: [PATCH 5/9] Adding test for perpetual_storage_wiggle_locality to take a list of localities --- fdbclient/FDBTypes.cpp | 128 ++++++++++++++++++ fdbclient/include/fdbclient/FDBTypes.h | 9 +- fdbserver/DDTeamCollection.actor.cpp | 55 -------- .../workloads/ConfigureDatabase.actor.cpp | 42 +++--- .../workloads/ConsistencyCheck.actor.cpp | 15 +- tests/SpecificUnitTest.txt | 3 +- 6 files changed, 166 insertions(+), 86 deletions(-) diff --git a/fdbclient/FDBTypes.cpp b/fdbclient/FDBTypes.cpp index 75fee4bbf3..107379659a 100644 --- a/fdbclient/FDBTypes.cpp +++ b/fdbclient/FDBTypes.cpp @@ -21,6 +21,7 @@ #include "fdbclient/FDBTypes.h" #include "fdbclient/Knobs.h" #include "fdbclient/NativeAPI.actor.h" +#include KeyRangeRef toPrefixRelativeRange(KeyRangeRef range, Optional prefix) { if (!prefix.present() || prefix.get().empty()) { @@ -261,5 +262,132 @@ TEST_CASE("/PerpetualStorageWiggleLocality/Validation") { ASSERT(!isValidPerpetualStorageWiggleLocality("aaa:bbb;")); ASSERT(!isValidPerpetualStorageWiggleLocality("aaa:bbb;ccc")); + return Void(); +} + +std::vector, Optional>> ParsePerpetualStorageWiggleLocality( + const std::string& localityKeyValues) { + // parsing format is like "datahall:0" + ASSERT(isValidPerpetualStorageWiggleLocality(localityKeyValues)); + + std::vector, Optional>> parsedLocalities; + + if (localityKeyValues == "0") { + return parsedLocalities; + } + + std::vector splitLocalityKeyValues; + boost::split(splitLocalityKeyValues, localityKeyValues, [](char c) { return c == ';'; }); + + for (const auto& localityKeyValue : splitLocalityKeyValues) { + ASSERT(!localityKeyValue.empty()); + + // get key and value from perpetual_storage_wiggle_locality. + int split = localityKeyValue.find(':'); + auto key = Optional(ValueRef((uint8_t*)localityKeyValue.c_str(), split)); + auto value = Optional( + ValueRef((uint8_t*)localityKeyValue.c_str() + split + 1, localityKeyValue.size() - split - 1)); + parsedLocalities.push_back(std::make_pair(key, value)); + } + + return parsedLocalities; +} + +bool localityMatchInList(const std::vector, Optional>>& localityKeyValues, + const LocalityData& locality) { + for (const auto& [localityKey, localityValue] : localityKeyValues) { + if (locality.get(localityKey.get()) == localityValue) { + return true; + } + } + return false; +} + +TEST_CASE("/PerpetualStorageWiggleLocality/ParsePerpetualStorageWiggleLocality") { + { + auto localityKeyValues = ParsePerpetualStorageWiggleLocality("aaa:bbb"); + ASSERT(localityKeyValues.size() == 1); + ASSERT(localityKeyValues[0].first.get() == "aaa"); + ASSERT(localityKeyValues[0].second.get() == "bbb"); + + { + LocalityData locality; + locality.set("aaa"_sr, "bbb"_sr); + ASSERT(localityMatchInList(localityKeyValues, locality)); + } + + { + LocalityData locality; + locality.set("aaa"_sr, "ccc"_sr); + ASSERT(!localityMatchInList(localityKeyValues, locality)); + } + } + + { + auto localityKeyValues = ParsePerpetualStorageWiggleLocality("aaa:bbb;ccc:ddd"); + ASSERT(localityKeyValues.size() == 2); + ASSERT(localityKeyValues[0].first.get() == "aaa"); + ASSERT(localityKeyValues[0].second.get() == "bbb"); + ASSERT(localityKeyValues[1].first.get() == "ccc"); + ASSERT(localityKeyValues[1].second.get() == "ddd"); + + { + LocalityData locality; + locality.set("aaa"_sr, "bbb"_sr); + ASSERT(localityMatchInList(localityKeyValues, locality)); + } + + { + LocalityData locality; + locality.set("ccc"_sr, "ddd"_sr); + ASSERT(localityMatchInList(localityKeyValues, locality)); + } + + { + LocalityData locality; + locality.set("aaa"_sr, "ddd"_sr); + ASSERT(!localityMatchInList(localityKeyValues, locality)); + } + } + + { + auto localityKeyValues = ParsePerpetualStorageWiggleLocality("aaa:111;bbb:222;ccc:3dd"); + ASSERT(localityKeyValues.size() == 3); + ASSERT(localityKeyValues[0].first.get() == "aaa"); + ASSERT(localityKeyValues[0].second.get() == "111"); + ASSERT(localityKeyValues[1].first.get() == "bbb"); + ASSERT(localityKeyValues[1].second.get() == "222"); + ASSERT(localityKeyValues[2].first.get() == "ccc"); + ASSERT(localityKeyValues[2].second.get() == "3dd"); + + { + LocalityData locality; + locality.set("aaa"_sr, "111"_sr); + ASSERT(localityMatchInList(localityKeyValues, locality)); + } + + { + LocalityData locality; + locality.set("bbb"_sr, "222"_sr); + ASSERT(localityMatchInList(localityKeyValues, locality)); + } + + { + LocalityData locality; + locality.set("ccc"_sr, "222"_sr); + ASSERT(!localityMatchInList(localityKeyValues, locality)); + } + } + + { + auto localityKeyValues = ParsePerpetualStorageWiggleLocality("0"); + ASSERT(localityKeyValues.empty()); + + { + LocalityData locality; + locality.set("aaa"_sr, "111"_sr); + ASSERT(!localityMatchInList(localityKeyValues, locality)); + } + } return Void(); } \ No newline at end of file diff --git a/fdbclient/include/fdbclient/FDBTypes.h b/fdbclient/include/fdbclient/FDBTypes.h index 331982623f..cdfdad90e1 100644 --- a/fdbclient/include/fdbclient/FDBTypes.h +++ b/fdbclient/include/fdbclient/FDBTypes.h @@ -36,6 +36,7 @@ #include "flow/ProtocolVersion.h" #include "flow/flow.h" #include "fdbclient/Status.h" +#include "fdbrpc/Locality.h" typedef int64_t Version; typedef uint64_t LogEpoch; @@ -1615,12 +1616,18 @@ struct DatabaseSharedState { const static std::regex wiggleLocalityValidation("(\\w+:\\w+)(;\\w+:\\w+)*"); inline bool isValidPerpetualStorageWiggleLocality(std::string locality) { - if (locality == "0") { + if (locality == "0") { return true; } return std::regex_match(locality, wiggleLocalityValidation); } +std::vector, Optional>> ParsePerpetualStorageWiggleLocality( + const std::string& localityKeyValues); + +bool localityMatchInList(const std::vector, Optional>>& localityKeyValues, + const LocalityData& locality); + // matches what's in fdb_c.h struct ReadBlobGranuleContext { // User context to pass along to functions diff --git a/fdbserver/DDTeamCollection.actor.cpp b/fdbserver/DDTeamCollection.actor.cpp index d18836e075..ee0c1c282c 100644 --- a/fdbserver/DDTeamCollection.actor.cpp +++ b/fdbserver/DDTeamCollection.actor.cpp @@ -27,7 +27,6 @@ #include "flow/actorcompiler.h" // This must be the last #include. #include "flow/network.h" #include -#include namespace { @@ -39,60 +38,6 @@ auto get(MapContainer& m, K const& k) -> decltype(m.at(k)) { return it->second; } -std::vector, Optional>> ParsePerpetualStorageWiggleLocality( - const std::string& localityKeyValues) { - // parsing format is like "datahall:0" - ASSERT(isValidPerpetualStorageWiggleLocality(localityKeyValues)); - - std::vector, Optional>> parsedLocalities; - - std::vector splitLocalityKeyValues; - boost::split(splitLocalityKeyValues, localityKeyValues, [](char c) { return c == ';'; }); - - for (const auto& localityKeyValue : splitLocalityKeyValues) { - ASSERT(!localityKeyValue.empty()); - - // get key and value from perpetual_storage_wiggle_locality. - int split = localityKeyValue.find(':'); - auto key = Optional(ValueRef((uint8_t*)localityKeyValue.c_str(), split)); - auto value = Optional( - ValueRef((uint8_t*)localityKeyValue.c_str() + split + 1, localityKeyValue.size() - split - 1)); - parsedLocalities.push_back(std::make_pair(key, value)); - } - - return parsedLocalities; -} - -TEST_CASE("/DataDistribution/StorageWiggler/ParsePerpetualStorageWiggleLocality") { - { - auto localityKeyValues = ParsePerpetualStorageWiggleLocality("aaa:bbb"); - ASSERT(localityKeyValues.size() == 1); - ASSERT(localityKeyValues[0].first.get() == "aaa"); - ASSERT(localityKeyValues[0].second.get() == "bbb"); - } - - { - auto localityKeyValues = ParsePerpetualStorageWiggleLocality("aaa:bbb;ccc:ddd"); - ASSERT(localityKeyValues.size() == 2); - ASSERT(localityKeyValues[0].first.get() == "aaa"); - ASSERT(localityKeyValues[0].second.get() == "bbb"); - ASSERT(localityKeyValues[1].first.get() == "ccc"); - ASSERT(localityKeyValues[1].second.get() == "ddd"); - } - - { - auto localityKeyValues = ParsePerpetualStorageWiggleLocality("aaa:111;bbb:222;ccc:3dd"); - ASSERT(localityKeyValues.size() == 3); - ASSERT(localityKeyValues[0].first.get() == "aaa"); - ASSERT(localityKeyValues[0].second.get() == "111"); - ASSERT(localityKeyValues[1].first.get() == "bbb"); - ASSERT(localityKeyValues[1].second.get() == "222"); - ASSERT(localityKeyValues[2].first.get() == "ccc"); - ASSERT(localityKeyValues[2].second.get() == "3dd"); - } - return Void(); -} - } // namespace namespace data_distribution { diff --git a/fdbserver/workloads/ConfigureDatabase.actor.cpp b/fdbserver/workloads/ConfigureDatabase.actor.cpp index 7b6283c4b5..3189f9c1df 100644 --- a/fdbserver/workloads/ConfigureDatabase.actor.cpp +++ b/fdbserver/workloads/ConfigureDatabase.actor.cpp @@ -18,6 +18,8 @@ * limitations under the License. */ +#include + #include "fdbclient/FDBTypes.h" #include "fdbclient/NativeAPI.actor.h" #include "fdbserver/TesterInterface.actor.h" @@ -305,14 +307,9 @@ struct ConfigureDatabaseWorkload : TestWorkload { state DatabaseConfiguration conf = wait(getDatabaseConfiguration(cx)); state std::string wiggleLocalityKeyValue = conf.perpetualStorageWiggleLocality; - state std::string wiggleLocalityKey; - state std::string wiggleLocalityValue; + state std::vector, Optional>> wiggleLocalityKeyValues = + ParsePerpetualStorageWiggleLocality(wiggleLocalityKeyValue); state int i; - if (wiggleLocalityKeyValue != "0") { - int split = wiggleLocalityKeyValue.find(':'); - wiggleLocalityKey = wiggleLocalityKeyValue.substr(0, split); - wiggleLocalityValue = wiggleLocalityKeyValue.substr(split + 1); - } state bool pass = true; state std::vector storageServers = wait(getStorageServers(cx)); @@ -321,8 +318,7 @@ struct ConfigureDatabaseWorkload : TestWorkload { // Check that each storage server has the correct key value store type if (!storageServers[i].isTss() && (wiggleLocalityKeyValue == "0" || - (storageServers[i].locality.get(wiggleLocalityKey).present() && - storageServers[i].locality.get(wiggleLocalityKey).get().toString() == wiggleLocalityValue))) { + localityMatchInList(wiggleLocalityKeyValues, storageServers[i].locality))) { ReplyPromise typeReply; ErrorOr keyValueStoreType = wait(storageServers[i].getKeyValueStoreType.getReplyUnlessFailedFor(typeReply, 2, 0)); @@ -478,19 +474,31 @@ struct ConfigureDatabaseWorkload : TestWorkload { state std::string randomPerpetualWiggleLocality; if (deterministicRandom()->random01() < 0.25) { state std::vector storageServers = wait(getStorageServers(cx)); - StorageServerInterface randomSS = - storageServers[deterministicRandom()->randomInt(0, storageServers.size())]; + std::string localityFilter; + int selectSSCount = + deterministicRandom()->randomInt(1, std::min(4, (int)(storageServers.size()))); std::vector localityKeys = { LocalityData::keyDcId, LocalityData::keyDataHallId, LocalityData::keyZoneId, LocalityData::keyMachineId, LocalityData::keyProcessId }; - StringRef randomLocalityKey = - localityKeys[deterministicRandom()->randomInt(0, localityKeys.size())]; - if (randomSS.locality.isPresent(randomLocalityKey)) { - randomPerpetualWiggleLocality = - " perpetual_storage_wiggle_locality=" + randomLocalityKey.toString() + ":" + - randomSS.locality.get(randomLocalityKey).get().toString(); + for (int i = 0; i < selectSSCount; ++i) { + StorageServerInterface randomSS = + storageServers[deterministicRandom()->randomInt(0, storageServers.size())]; + StringRef randomLocalityKey = + localityKeys[deterministicRandom()->randomInt(0, localityKeys.size())]; + if (randomSS.locality.isPresent(randomLocalityKey)) { + if (localityFilter.size() > 0) { + localityFilter += ";"; + } + localityFilter += randomLocalityKey.toString() + ":" + + randomSS.locality.get(randomLocalityKey).get().toString(); + } + } + + if (localityFilter.size() > 0) { + TraceEvent("ConfigureTestSettingWiggleLocality").detail("LocalityFilter", localityFilter); + randomPerpetualWiggleLocality = " perpetual_storage_wiggle_locality=" + localityFilter; } } diff --git a/fdbserver/workloads/ConsistencyCheck.actor.cpp b/fdbserver/workloads/ConsistencyCheck.actor.cpp index c1c6a2c381..bfd799532f 100644 --- a/fdbserver/workloads/ConsistencyCheck.actor.cpp +++ b/fdbserver/workloads/ConsistencyCheck.actor.cpp @@ -931,13 +931,8 @@ struct ConsistencyCheckWorkload : TestWorkload { state int j; state std::vector storageServers = wait(getStorageServers(cx)); state std::string wiggleLocalityKeyValue = configuration.perpetualStorageWiggleLocality; - state std::string wiggleLocalityKey; - state std::string wiggleLocalityValue; - if (wiggleLocalityKeyValue != "0") { - int split = wiggleLocalityKeyValue.find(':'); - wiggleLocalityKey = wiggleLocalityKeyValue.substr(0, split); - wiggleLocalityValue = wiggleLocalityKeyValue.substr(split + 1); - } + state std::vector, Optional>> wiggleLocalityKeyValues = + ParsePerpetualStorageWiggleLocality(configuration.perpetualStorageWiggleLocality); // Check each pair of storage servers for an address match for (i = 0; i < storageServers.size(); i++) { @@ -953,8 +948,7 @@ struct ConsistencyCheckWorkload : TestWorkload { // Perpetual storage wiggle is used to migrate storage. Check that the matched storage servers are // correctly migrated. if (wiggleLocalityKeyValue == "0" || - (storageServers[i].locality.get(wiggleLocalityKey).present() && - storageServers[i].locality.get(wiggleLocalityKey).get().toString() == wiggleLocalityValue)) { + localityMatchInList(wiggleLocalityKeyValues, storageServers[i].locality)) { if (keyValueStoreType.get() != configuration.perpetualStoreType) { TraceEvent("ConsistencyCheck_WrongKeyValueStoreType") .detail("ServerID", storageServers[i].id()) @@ -981,8 +975,7 @@ struct ConsistencyCheckWorkload : TestWorkload { (storageServers[i].isTss() && keyValueStoreType.get() != configuration.testingStorageServerStoreType)) && (wiggleLocalityKeyValue == "0" || - (storageServers[i].locality.get(wiggleLocalityKey).present() && - storageServers[i].locality.get(wiggleLocalityKey).get().toString() == wiggleLocalityValue))) { + localityMatchInList(wiggleLocalityKeyValues, storageServers[i].locality))) { TraceEvent("ConsistencyCheck_WrongKeyValueStoreType") .detail("ServerID", storageServers[i].id()) .detail("StoreType", keyValueStoreType.get().toString()) diff --git a/tests/SpecificUnitTest.txt b/tests/SpecificUnitTest.txt index f3405d79d5..0a8e2e2eab 100644 --- a/tests/SpecificUnitTest.txt +++ b/tests/SpecificUnitTest.txt @@ -5,5 +5,4 @@ runSetup=false testName=UnitTests maxTestCases=0 - testsMatching=/DataDistribution/StorageWiggler/ParsePerpetualStorageWiggleLocality - # testsMatching=/PerpetualStorageWiggleLocality/Validation + testsMatching=/status/json/builder From bebd1790db2517fb579158d507782a51bb41ffc4 Mon Sep 17 00:00:00 2001 From: Zhe Wu Date: Wed, 13 Sep 2023 14:28:21 -0700 Subject: [PATCH 6/9] Resolve conflict with recent change in main --- fdbserver/DDTeamCollection.actor.cpp | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/fdbserver/DDTeamCollection.actor.cpp b/fdbserver/DDTeamCollection.actor.cpp index ee0c1c282c..f2d4cc4caa 100644 --- a/fdbserver/DDTeamCollection.actor.cpp +++ b/fdbserver/DDTeamCollection.actor.cpp @@ -3005,21 +3005,22 @@ public: // SOMEDAY: support wiggle multiple SS at once ASSERT(!self->wigglingId.present()); // only single process wiggle is allowed - Optional localityKey; - Optional localityValue; + std::vector, Optional>> localityKeyValues; if (self->configuration.perpetualStorageWiggleLocality != "0") { - ParsePerpetualStorageWiggleLocality( - self->configuration.perpetualStorageWiggleLocality, &localityKey, &localityValue); + localityKeyValues = + ParsePerpetualStorageWiggleLocality(self->configuration.perpetualStorageWiggleLocality); } // if perpetual_storage_wiggle_locality has value and not 0(disabled). - if (localityKey.present()) { - if (self->server_info.count(res.begin()->first)) { - auto server = self->server_info.at(res.begin()->first); + if (!localityKeyValues.empty()) { + for (const auto& [localityKey, localityValue] : localityKeyValues) { + if (self->server_info.count(res.begin()->first)) { + auto server = self->server_info.at(res.begin()->first); - // Update the wigglingId only if it matches the locality. - if (server->getLastKnownInterface().locality.get(localityKey.get()) == localityValue) { - self->wigglingId = res.begin()->first; + // Update the wigglingId only if it matches the locality. + if (server->getLastKnownInterface().locality.get(localityKey.get()) == localityValue) { + self->wigglingId = res.begin()->first; + } } } } else { From dcb0ccbd37afb48fe4f4c3be79c83559cc3f43b6 Mon Sep 17 00:00:00 2001 From: Zhe Wu Date: Wed, 13 Sep 2023 14:37:02 -0700 Subject: [PATCH 7/9] Adding more test; adding documentations --- fdbclient/FDBTypes.cpp | 3 ++- fdbclient/include/fdbclient/FDBTypes.h | 3 +++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/fdbclient/FDBTypes.cpp b/fdbclient/FDBTypes.cpp index 107379659a..a6a945aae3 100644 --- a/fdbclient/FDBTypes.cpp +++ b/fdbclient/FDBTypes.cpp @@ -261,13 +261,14 @@ TEST_CASE("/PerpetualStorageWiggleLocality/Validation") { ASSERT(!isValidPerpetualStorageWiggleLocality("aaa:bbb;")); ASSERT(!isValidPerpetualStorageWiggleLocality("aaa:bbb;ccc")); + ASSERT(!isValidPerpetualStorageWiggleLocality("")); return Void(); } std::vector, Optional>> ParsePerpetualStorageWiggleLocality( const std::string& localityKeyValues) { - // parsing format is like "datahall:0" + // parsing format is like "datahall:0<;locality:filter>" ASSERT(isValidPerpetualStorageWiggleLocality(localityKeyValues)); std::vector, Optional>> parsedLocalities; diff --git a/fdbclient/include/fdbclient/FDBTypes.h b/fdbclient/include/fdbclient/FDBTypes.h index cdfdad90e1..44e76b895f 100644 --- a/fdbclient/include/fdbclient/FDBTypes.h +++ b/fdbclient/include/fdbclient/FDBTypes.h @@ -1622,9 +1622,12 @@ inline bool isValidPerpetualStorageWiggleLocality(std::string locality) { return std::regex_match(locality, wiggleLocalityValidation); } +// Parses `perpetual_storage_wiggle_locality` database option. std::vector, Optional>> ParsePerpetualStorageWiggleLocality( const std::string& localityKeyValues); +// Whether the locality matches any locality filter in `localityKeyValues` (which is supposed to be parsed from +// ParsePerpetualStorageWiggleLocality). bool localityMatchInList(const std::vector, Optional>>& localityKeyValues, const LocalityData& locality); From 31d46b6fb2c4212a1ecb9c687190c8f4a4e03f88 Mon Sep 17 00:00:00 2001 From: Zhe Wu Date: Tue, 19 Sep 2023 10:35:51 -0700 Subject: [PATCH 8/9] Address comments --- fdbserver/DDTeamCollection.actor.cpp | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/fdbserver/DDTeamCollection.actor.cpp b/fdbserver/DDTeamCollection.actor.cpp index f2d4cc4caa..fae33244c5 100644 --- a/fdbserver/DDTeamCollection.actor.cpp +++ b/fdbserver/DDTeamCollection.actor.cpp @@ -2406,11 +2406,8 @@ public: } else { std::vector, Optional>> localityKeyValues = ParsePerpetualStorageWiggleLocality(self->configuration.perpetualStorageWiggleLocality); - for (const auto& [localityKey, localityValue] : localityKeyValues) { - if (candidateWorker.worker.locality.get(localityKey.get()) == localityValue) { - isr.storeType = self->configuration.perpetualStoreType; - break; - } + if (localityMatchInList(localityKeyValues, candidateWorker.worker.locality)) { + isr.storeType = self->configuration.perpetualStoreType; } } } @@ -2976,12 +2973,9 @@ public: if (!localityKeyValues.empty()) { // Whether the selected server matches the locality auto server = teamCollection->server_info.at(id.get()); - - for (const auto& [localityKey, localityValue] : localityKeyValues) { - // TraceEvent("PerpetualLocality").detail("Server", server->getLastKnownInterface().locality.get(localityKey)).detail("Desire", localityValue); - if (server->getLastKnownInterface().locality.get(localityKey.get()) == localityValue) { - return id.get(); - } + // TraceEvent("PerpetualLocality").detail("Server", server->getLastKnownInterface().locality.get(localityKey)).detail("Desire", localityValue); + if (localityMatchInList(localityKeyValues, server->getLastKnownInterface().locality)) { + return id.get(); } if (wiggler->empty()) { From d65a6a8a10597a77e7bc6249fc590f7319116033 Mon Sep 17 00:00:00 2001 From: Zhe Wu Date: Wed, 20 Sep 2023 12:55:15 -0700 Subject: [PATCH 9/9] Address comments --- fdbserver/DDTeamCollection.actor.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/fdbserver/DDTeamCollection.actor.cpp b/fdbserver/DDTeamCollection.actor.cpp index fae33244c5..e8cb537f3b 100644 --- a/fdbserver/DDTeamCollection.actor.cpp +++ b/fdbserver/DDTeamCollection.actor.cpp @@ -3007,10 +3007,9 @@ public: // if perpetual_storage_wiggle_locality has value and not 0(disabled). if (!localityKeyValues.empty()) { - for (const auto& [localityKey, localityValue] : localityKeyValues) { - if (self->server_info.count(res.begin()->first)) { - auto server = self->server_info.at(res.begin()->first); - + if (self->server_info.count(res.begin()->first)) { + auto server = self->server_info.at(res.begin()->first); + for (const auto& [localityKey, localityValue] : localityKeyValues) { // Update the wigglingId only if it matches the locality. if (server->getLastKnownInterface().locality.get(localityKey.get()) == localityValue) { self->wigglingId = res.begin()->first;