From ec307b3f2c016794e575d9c94e8d3a133e393fc9 Mon Sep 17 00:00:00 2001 From: Vaidas Gasiunas Date: Wed, 20 Oct 2021 17:19:12 +0200 Subject: [PATCH] MVC2.0: Addressing code review comments for client lib management operations --- fdbclient/ClientLibManagement.actor.cpp | 128 ++++++++++-------- fdbclient/ClientLibManagement.actor.h | 49 +++---- fdbserver/tester.actor.cpp | 30 ++-- .../ClientLibManagementWorkload.actor.cpp | 36 ++--- fdbserver/workloads/workloads.actor.h | 9 +- 5 files changed, 133 insertions(+), 119 deletions(-) diff --git a/fdbclient/ClientLibManagement.actor.cpp b/fdbclient/ClientLibManagement.actor.cpp index 01da8b4504..58d27a361f 100644 --- a/fdbclient/ClientLibManagement.actor.cpp +++ b/fdbclient/ClientLibManagement.actor.cpp @@ -43,16 +43,20 @@ struct ClientLibBinaryInfo { Standalone sumBytes; }; +#define ASSERT_INDEX_IN_RANGE(idx, arr) ASSERT(idx >= 0 && idx < sizeof(arr) / sizeof(arr[0])) + const std::string& getStatusName(ClientLibStatus status) { static const std::string statusNames[] = { "disabled", "available", "uploading", "deleting" }; - return statusNames[status]; + int idx = static_cast(status); + ASSERT_INDEX_IN_RANGE(idx, statusNames); + return statusNames[idx]; } ClientLibStatus getStatusByName(std::string_view statusName) { static std::map statusByName; // initialize the map on demand if (statusByName.empty()) { - for (int i = 0; i < CLIENTLIB_STATUS_COUNT; i++) { + for (int i = 0; i < static_cast(ClientLibStatus::COUNT); i++) { ClientLibStatus status = static_cast(i); statusByName[getStatusName(status)] = status; } @@ -68,14 +72,16 @@ ClientLibStatus getStatusByName(std::string_view statusName) { const std::string& getPlatformName(ClientLibPlatform platform) { static const std::string platformNames[] = { "unknown", "x84_64-linux", "x86_64-windows", "x86_64-macos" }; - return platformNames[platform]; + int idx = static_cast(platform); + ASSERT_INDEX_IN_RANGE(idx, platformNames); + return platformNames[idx]; } ClientLibPlatform getPlatformByName(std::string_view platformName) { static std::map platformByName; // initialize the map on demand if (platformByName.empty()) { - for (int i = 0; i < CLIENTLIB_PLATFORM_COUNT; i++) { + for (int i = 0; i < static_cast(ClientLibStatus::COUNT); i++) { ClientLibPlatform platform = static_cast(i); platformByName[getPlatformName(platform)] = platform; } @@ -91,14 +97,16 @@ ClientLibPlatform getPlatformByName(std::string_view platformName) { const std::string& getChecksumAlgName(ClientLibChecksumAlg checksumAlg) { static const std::string checksumAlgNames[] = { "md5" }; - return checksumAlgNames[checksumAlg]; + int idx = static_cast(checksumAlg); + ASSERT_INDEX_IN_RANGE(idx, checksumAlgNames); + return checksumAlgNames[idx]; } ClientLibChecksumAlg getChecksumAlgByName(std::string_view checksumAlgName) { static std::map checksumAlgByName; // initialize the map on demand if (checksumAlgByName.empty()) { - for (int i = 0; i < CLIENTLIB_CHECKSUM_ALG_COUNT; i++) { + for (int i = 0; i < (int)ClientLibChecksumAlg::COUNT; i++) { ClientLibChecksumAlg checksumAlg = static_cast(i); checksumAlgByName[getChecksumAlgName(checksumAlg)] = checksumAlg; } @@ -115,7 +123,7 @@ ClientLibChecksumAlg getChecksumAlgByName(std::string_view checksumAlgName) { namespace { bool isValidTargetStatus(ClientLibStatus status) { - return status == CLIENTLIB_AVAILABLE || status == CLIENTLIB_DISABLED; + return status == ClientLibStatus::AVAILABLE || status == ClientLibStatus::DISABLED; } void parseMetadataJson(StringRef metadataString, json_spirit::mObject& metadataJson) { @@ -157,8 +165,10 @@ bool validVersionPartNum(int num) { int getNumericVersionEncoding(const std::string& versionStr) { int major, minor, patch; - int numScanned = sscanf(versionStr.c_str(), "%d.%d.%d", &major, &minor, &patch); - if (numScanned != 3 || !validVersionPartNum(major) || !validVersionPartNum(minor) || !validVersionPartNum(patch)) { + int charsScanned; + int numScanned = sscanf(versionStr.c_str(), "%d.%d.%d%n", &major, &minor, &patch, &charsScanned); + if (numScanned != 3 || !validVersionPartNum(major) || !validVersionPartNum(minor) || !validVersionPartNum(patch) || + charsScanned != versionStr.size()) { TraceEvent(SevWarnAlways, "ClientLibraryInvalidMetadata") .detail("Error", format("Invalid version string %s", versionStr.c_str())); throw client_lib_invalid_metadata(); @@ -166,22 +176,22 @@ int getNumericVersionEncoding(const std::string& versionStr) { return ((major * 1000) + minor) * 1000 + patch; } -void getIdFromMetadataJson(const json_spirit::mObject& metadataJson, std::string& clientLibId) { +Standalone getIdFromMetadataJson(const json_spirit::mObject& metadataJson) { std::ostringstream libIdBuilder; libIdBuilder << getMetadataStrAttr(metadataJson, CLIENTLIB_ATTR_PLATFORM) << "/"; libIdBuilder << format("%09d", getNumericVersionEncoding(getMetadataStrAttr(metadataJson, CLIENTLIB_ATTR_VERSION))) << "/"; libIdBuilder << getMetadataStrAttr(metadataJson, CLIENTLIB_ATTR_TYPE) << "/"; libIdBuilder << getMetadataStrAttr(metadataJson, CLIENTLIB_ATTR_CHECKSUM); - clientLibId = libIdBuilder.str(); + return Standalone(libIdBuilder.str()); } -Key metadataKeyFromId(const std::string& clientLibId) { - return StringRef(clientLibId).withPrefix(clientLibMetadataPrefix); +Key metadataKeyFromId(StringRef clientLibId) { + return clientLibId.withPrefix(clientLibMetadataPrefix); } -Key chunkKeyPrefixFromId(const std::string& clientLibId) { - return StringRef(clientLibId).withPrefix(clientLibBinaryPrefix).withSuffix(LiteralStringRef("/")); +Key chunkKeyPrefixFromId(StringRef clientLibId) { + return clientLibId.withPrefix(clientLibBinaryPrefix).withSuffix(LiteralStringRef("/")); } KeyRef chunkKeyFromNo(StringRef clientLibBinPrefix, size_t chunkNo, Arena& arena) { @@ -191,16 +201,16 @@ KeyRef chunkKeyFromNo(StringRef clientLibBinPrefix, size_t chunkNo, Arena& arena ClientLibPlatform getCurrentClientPlatform() { #ifdef __x86_64__ #if defined(_WIN32) - return CLIENTLIB_X86_64_WINDOWS; + return ClientLibPlatform::X86_64_WINDOWS; #elif defined(__linux__) - return CLIENTLIB_X86_64_LINUX; + return ClientLibPlatform::X86_64_LINUX; #elif defined(__FreeBSD__) || defined(__APPLE__) - return CLIENTLIB_X86_64_MACOS; + return ClientLibPlatform::X86_64_MACOS; #else - return CLIENTLIB_UNKNOWN_PLATFORM; + return ClientLibPlatform::UNKNOWN; #endif #else // not __x86_64__ - return CLIENTLIB_UNKNOWN_PLATFORM; + return ClientLibPlatform::UNKNOWN; #endif } @@ -217,7 +227,7 @@ Standalone byteArrayToHexString(StringRef input) { } // namespace -Standalone MD5SumToHexString(MD5_CTX& sum) { +Standalone md5SumToHexString(MD5_CTX& sum) { Standalone sumBytes = makeString(16); ::MD5_Final(mutateString(sumBytes), &sum); return byteArrayToHexString(sumBytes); @@ -229,10 +239,10 @@ ClientLibFilter& ClientLibFilter::filterNewerPackageVersion(const std::string& v return *this; } -void getClientLibIdFromMetadataJson(StringRef metadataString, std::string& clientLibId) { +Standalone getClientLibIdFromMetadataJson(StringRef metadataString) { json_spirit::mObject parsedMetadata; parseMetadataJson(metadataString, parsedMetadata); - getIdFromMetadataJson(parsedMetadata, clientLibId); + return getIdFromMetadataJson(parsedMetadata); } namespace { @@ -253,8 +263,9 @@ ACTOR Future uploadClientLibBinary(Database db, state size_t firstChunkNo; if (transactionSize % chunkSize != 0) { - TraceEvent(SevError, "ClientLibraryInvalidConfig") - .detail("Reason", format("Invalid chunk size configuration, falling back to %d", _PAGE_SIZE)); + TraceEvent(SevWarnAlways, "ClientLibraryInvalidConfig") + .detail("Reason", + format("Invalid chunk size configuration (%d), falling back to %d", chunkSize, _PAGE_SIZE)); chunkSize = _PAGE_SIZE; } @@ -303,7 +314,7 @@ ACTOR Future uploadClientLibBinary(Database db, binInfo->totalBytes = fileOffset; binInfo->chunkCnt = chunkNo; binInfo->chunkSize = chunkSize; - binInfo->sumBytes = MD5SumToHexString(sum); + binInfo->sumBytes = md5SumToHexString(sum); return Void(); } @@ -354,9 +365,11 @@ ACTOR Future deleteClientLibMetadataEntry(Database db, Key clientLibMetaKe } // namespace -ACTOR Future uploadClientLibrary(Database db, StringRef metadataString, StringRef libFilePath) { +ACTOR Future uploadClientLibrary(Database db, + Standalone metadataString, + Standalone libFilePath) { state json_spirit::mObject metadataJson; - state std::string clientLibId; + state Standalone clientLibId; state Key clientLibMetaKey; state Key clientLibBinPrefix; state std::string jsStr; @@ -380,7 +393,7 @@ ACTOR Future uploadClientLibrary(Database db, StringRef metadataString, St throw client_lib_invalid_metadata(); } - getIdFromMetadataJson(metadataJson, clientLibId); + clientLibId = getIdFromMetadataJson(metadataJson); clientLibMetaKey = metadataKeyFromId(clientLibId); clientLibBinPrefix = chunkKeyPrefixFromId(clientLibId); @@ -401,7 +414,7 @@ ACTOR Future uploadClientLibrary(Database db, StringRef metadataString, St getMetadataStrAttr(metadataJson, CLIENTLIB_ATTR_PROTOCOL); getMetadataIntAttr(metadataJson, CLIENTLIB_ATTR_API_VERSION); - metadataJson[CLIENTLIB_ATTR_STATUS] = getStatusName(CLIENTLIB_UPLOADING); + metadataJson[CLIENTLIB_ATTR_STATUS] = getStatusName(ClientLibStatus::UPLOADING); jsStr = json_spirit::write_string(json_spirit::mValue(metadataJson)); /* @@ -426,9 +439,6 @@ ACTOR Future uploadClientLibrary(Database db, StringRef metadataString, St wait(tr.commit()); break; } catch (Error& e) { - if (e.code() == error_code_client_lib_already_exists) { - throw; - } wait(tr.onError(e)); } } @@ -481,9 +491,11 @@ ACTOR Future uploadClientLibrary(Database db, StringRef metadataString, St return Void(); } -ACTOR Future downloadClientLibrary(Database db, StringRef clientLibId, StringRef libFilePath) { - state Key clientLibMetaKey = metadataKeyFromId(clientLibId.toString()); - state Key chunkKeyPrefix = chunkKeyPrefixFromId(clientLibId.toString()); +ACTOR Future downloadClientLibrary(Database db, + Standalone clientLibId, + Standalone libFilePath) { + state Key clientLibMetaKey = metadataKeyFromId(clientLibId); + state Key chunkKeyPrefix = chunkKeyPrefixFromId(clientLibId); state int transactionSize = getAlignedUpperBound(CLIENT_KNOBS->MVC_CLIENTLIB_TRANSACTION_SIZE, _PAGE_SIZE); state json_spirit::mObject metadataJson; state std::string checkSum; @@ -518,15 +530,12 @@ ACTOR Future downloadClientLibrary(Database db, StringRef clientLibId, Str parseMetadataJson(metadataOpt.get(), metadataJson); break; } catch (Error& e) { - if (e.code() == error_code_client_lib_not_found) { - throw; - } wait(tr.onError(e)); } } // Allow downloading only libraries in the available state - if (getStatusByName(getMetadataStrAttr(metadataJson, CLIENTLIB_ATTR_STATUS)) != CLIENTLIB_AVAILABLE) { + if (getStatusByName(getMetadataStrAttr(metadataJson, CLIENTLIB_ATTR_STATUS)) != ClientLibStatus::AVAILABLE) { throw client_lib_not_available(); } @@ -540,7 +549,20 @@ ACTOR Future downloadClientLibrary(Database db, StringRef clientLibId, Str chunkCount = getMetadataIntAttr(metadataJson, CLIENTLIB_ATTR_CHUNK_COUNT); binarySize = getMetadataIntAttr(metadataJson, CLIENTLIB_ATTR_SIZE); expectedChunkSize = getMetadataIntAttr(metadataJson, CLIENTLIB_ATTR_CHUNK_SIZE); - ASSERT(transactionSize % expectedChunkSize == 0); + + if (transactionSize % expectedChunkSize != 0) { + // Make sure the transaction size alignes both by chunk size and the page size + int fixedSize = getAlignedUpperBound(transactionSize, std::lcm((int)expectedChunkSize, _PAGE_SIZE)); + TraceEvent(SevWarnAlways, "ClientLibraryInvalidConfig") + .detail("Reason", + format("Configured transaction size %d does not align " + "with the stored chunk size%zu, falling back to %d", + transactionSize, + expectedChunkSize, + fixedSize)); + // make sure transactionSize assiz + transactionSize = fixedSize; + } fileOffset = 0; chunkNo = 0; @@ -591,9 +613,6 @@ ACTOR Future downloadClientLibrary(Database db, StringRef clientLibId, Str } break; } catch (Error& e) { - if (e.code() == error_code_client_lib_invalid_binary) { - throw; - } wait(tr.onError(e)); } } @@ -612,7 +631,7 @@ ACTOR Future downloadClientLibrary(Database db, StringRef clientLibId, Str throw client_lib_invalid_binary(); } - Standalone sumBytesStr = MD5SumToHexString(sum); + Standalone sumBytesStr = md5SumToHexString(sum); if (sumBytesStr != StringRef(checkSum)) { TraceEvent(SevWarnAlways, "ClientLibraryChecksumMismatch") .detail("Expected", checkSum) @@ -627,7 +646,7 @@ ACTOR Future downloadClientLibrary(Database db, StringRef clientLibId, Str return Void(); } -ACTOR Future deleteClientLibrary(Database db, StringRef clientLibId) { +ACTOR Future deleteClientLibrary(Database db, Standalone clientLibId) { state Key clientLibMetaKey = metadataKeyFromId(clientLibId.toString()); state Key chunkKeyPrefix = chunkKeyPrefixFromId(clientLibId.toString()); state json_spirit::mObject metadataJson; @@ -649,15 +668,12 @@ ACTOR Future deleteClientLibrary(Database db, StringRef clientLibId) { throw client_lib_not_found(); } parseMetadataJson(metadataOpt.get(), metadataJson); - metadataJson[CLIENTLIB_ATTR_STATUS] = getStatusName(CLIENTLIB_DELETING); + metadataJson[CLIENTLIB_ATTR_STATUS] = getStatusName(ClientLibStatus::DELETING); jsStr = json_spirit::write_string(json_spirit::mValue(metadataJson)); tr.set(clientLibMetaKey, ValueRef(jsStr)); wait(tr.commit()); break; } catch (Error& e) { - if (e.code() == error_code_client_lib_not_found) { - throw; - } wait(tr.onError(e)); } } @@ -679,8 +695,8 @@ void applyClientLibFilter(const ClientLibFilter& filter, try { json_spirit::mObject metadataJson; parseMetadataJson(v, metadataJson); - if (filter.matchAvailableOnly && - getStatusByName(getMetadataStrAttr(metadataJson, CLIENTLIB_ATTR_STATUS)) != CLIENTLIB_AVAILABLE) { + if (filter.matchAvailableOnly && getStatusByName(getMetadataStrAttr(metadataJson, CLIENTLIB_ATTR_STATUS)) != + ClientLibStatus::AVAILABLE) { continue; } if (filter.matchCompatibleAPI && @@ -724,7 +740,7 @@ ACTOR Future>> listClientLibraries(Database db, fromKey = fromKey.withSuffix(format("%09d", filter.numericPkgVersion + 1)); } toKey = prefixWithPlatform.withSuffix(LiteralStringRef("0")); - scanRange = KeyRangeRef(StringRef(fromKey), toKey); + scanRange = KeyRangeRef(fromKey, toKey); } else { scanRange = clientLibMetadataKeys; } @@ -744,10 +760,4 @@ ACTOR Future>> listClientLibraries(Database db, return result; } -Future>> listCompatibleClientLibraries(Database db, int apiVersion) { - return listClientLibraries( - db, - ClientLibFilter().filterAvailable().filterCompatibleAPI(apiVersion).filterPlatform(getCurrentClientPlatform())); -} - } // namespace ClientLibManagement \ No newline at end of file diff --git a/fdbclient/ClientLibManagement.actor.h b/fdbclient/ClientLibManagement.actor.h index eec2334542..88588eb698 100644 --- a/fdbclient/ClientLibManagement.actor.h +++ b/fdbclient/ClientLibManagement.actor.h @@ -33,27 +33,27 @@ namespace ClientLibManagement { -enum ClientLibStatus { - CLIENTLIB_DISABLED = 0, - CLIENTLIB_AVAILABLE, // 1 - CLIENTLIB_UPLOADING, // 2 - CLIENTLIB_DELETING, // 3 - CLIENTLIB_STATUS_COUNT // must be the last one +enum class ClientLibStatus { + DISABLED = 0, + AVAILABLE, // 1 + UPLOADING, // 2 + DELETING, // 3 + COUNT // must be the last one }; -enum ClientLibPlatform { - CLIENTLIB_UNKNOWN_PLATFORM = 0, - CLIENTLIB_X86_64_LINUX, - CLIENTLIB_X86_64_WINDOWS, - CLIENTLIB_X86_64_MACOS, - CLIENTLIB_PLATFORM_COUNT // must be the last one +enum class ClientLibPlatform { + UNKNOWN = 0, + X86_64_LINUX, + X86_64_WINDOWS, + X86_64_MACOS, + COUNT // must be the last one }; // Currently we support only one, // but we may want to change it in the future -enum ClientLibChecksumAlg { - CLIENTLIB_CHECKSUM_ALG_MD5 = 0, - CLIENTLIB_CHECKSUM_ALG_COUNT // must be the last one +enum class ClientLibChecksumAlg { + MD5 = 0, + COUNT // must be the last one }; inline const std::string CLIENTLIB_ATTR_PLATFORM{ "platform" }; @@ -75,7 +75,7 @@ struct ClientLibFilter { bool matchPlatform = false; bool matchCompatibleAPI = false; bool matchNewerPackageVersion = false; - ClientLibPlatform platformVal = CLIENTLIB_UNKNOWN_PLATFORM; + ClientLibPlatform platformVal = ClientLibPlatform::UNKNOWN; int apiVersion = 0; int numericPkgVersion = 0; @@ -110,29 +110,30 @@ const std::string& getChecksumAlgName(ClientLibChecksumAlg checksumAlg); ClientLibChecksumAlg getChecksumAlgByName(std::string_view checksumAlgName); // encodes MD5 result to a hexadecimal string to be provided in the checksum attribute -Standalone MD5SumToHexString(MD5_CTX& sum); +Standalone md5SumToHexString(MD5_CTX& sum); // Upload a client library binary from a file and associated metadata JSON // to the system keyspace of the database -ACTOR Future uploadClientLibrary(Database db, StringRef metadataString, StringRef libFilePath); +ACTOR Future uploadClientLibrary(Database db, + Standalone metadataString, + Standalone libFilePath); // Determine clientLibId from the relevant attributes of the metadata JSON -void getClientLibIdFromMetadataJson(StringRef metadataString, std::string& clientLibId); +Standalone getClientLibIdFromMetadataJson(StringRef metadataString); // Download a client library binary from the system keyspace of the database // and save it at the given file path -ACTOR Future downloadClientLibrary(Database db, StringRef clientLibId, StringRef libFilePath); +ACTOR Future downloadClientLibrary(Database db, + Standalone clientLibId, + Standalone libFilePath); // Delete the client library binary from to the system keyspace of the database -ACTOR Future deleteClientLibrary(Database db, StringRef clientLibId); +ACTOR Future deleteClientLibrary(Database db, Standalone clientLibId); // List client libraries available on the cluster, with the specified filter // Returns metadata JSON of each library ACTOR Future>> listClientLibraries(Database db, ClientLibFilter filter); -// List available client libraries that are compatible with the current client -Future>> listAvailableCompatibleClientLibraries(Database db, int apiVersion); - } // namespace ClientLibManagement #include "flow/unactorcompiler.h" diff --git a/fdbserver/tester.actor.cpp b/fdbserver/tester.actor.cpp index 431701d51b..4e43026a09 100644 --- a/fdbserver/tester.actor.cpp +++ b/fdbserver/tester.actor.cpp @@ -1707,10 +1707,10 @@ ACTOR Future runTests(Reference connFile, namespace { ACTOR Future testExpectedErrorImpl(Future test, const char* testDescr, - Error expectedError, - bool* successFlag, + Optional expectedError, + Optional successFlag, std::map details, - Error throwOnError, + Optional throwOnError, UID id) { state Error actualError; try { @@ -1721,19 +1721,21 @@ ACTOR Future testExpectedErrorImpl(Future test, } actualError = e; // The test failed as expected - if (!expectedError.isValid() || actualError.code() == expectedError.code()) { + if (!expectedError.present() || actualError.code() == expectedError.get().code()) { return Void(); } } // The test has failed - if (successFlag != nullptr) { - *successFlag = false; + if (successFlag.present()) { + *(successFlag.get()) = false; } TraceEvent evt(SevError, "TestErrorFailed", id); - evt.detail("TestDescr", testDescr); - evt.detail("ExpectedError", expectedError.name()); - evt.detail("ExpectedErrorCode", expectedError.code()); + evt.detail("TestDescription", testDescr); + if (expectedError.present()) { + evt.detail("ExpectedError", expectedError.get().name()); + evt.detail("ExpectedErrorCode", expectedError.get().code()); + } if (actualError.isValid()) { evt.detail("ActualError", actualError.name()); evt.detail("ActualErrorCode", actualError.code()); @@ -1743,8 +1745,8 @@ ACTOR Future testExpectedErrorImpl(Future test, for (auto& p : details) { evt.detail(p.first.c_str(), p.second); } - if (throwOnError.isValid()) { - throw throwOnError; + if (throwOnError.present()) { + throw throwOnError.get(); } return Void(); } @@ -1752,10 +1754,10 @@ ACTOR Future testExpectedErrorImpl(Future test, Future testExpectedError(Future test, const char* testDescr, - Error expectedError, - bool* successFlag, + Optional expectedError, + Optional successFlag, std::map details, - Error throwOnError, + Optional throwOnError, UID id) { return testExpectedErrorImpl(test, testDescr, expectedError, successFlag, details, throwOnError, id); } diff --git a/fdbserver/workloads/ClientLibManagementWorkload.actor.cpp b/fdbserver/workloads/ClientLibManagementWorkload.actor.cpp index 05482cfa24..f259f4c756 100644 --- a/fdbserver/workloads/ClientLibManagementWorkload.actor.cpp +++ b/fdbserver/workloads/ClientLibManagementWorkload.actor.cpp @@ -3,7 +3,7 @@ * * This source file is part of the FoundationDB open source project * - * Copyright 2013-2018 Apple Inc. and the FoundationDB project authors + * Copyright 2013-2021 Apple Inc. and the FoundationDB project authors * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -38,7 +38,7 @@ struct ClientLibManagementWorkload : public TestWorkload { size_t testFileSize = 0; RandomByteGenerator rbg; - std::string uploadedClientLibId; + Standalone uploadedClientLibId; json_spirit::mObject uploadedMetadataJson; Standalone generatedChecksum; std::string generatedFileName; @@ -91,7 +91,7 @@ struct ClientLibManagementWorkload : public TestWorkload { } wait(file->sync()); - self->generatedChecksum = MD5SumToHexString(sum); + self->generatedChecksum = md5SumToHexString(sum); return Void(); } @@ -138,7 +138,7 @@ struct ClientLibManagementWorkload : public TestWorkload { for (auto& testMetadataStr : invalidMetadataStrs) { metadataStr = StringRef(testMetadataStr); - wait(testExpectedError(uploadClientLibrary(cx, metadataStr, self->generatedFileName), + wait(testExpectedError(uploadClientLibrary(cx, metadataStr, StringRef(self->generatedFileName)), "uploadClientLibrary with invalid metadata", client_lib_invalid_metadata(), &self->success, @@ -164,8 +164,8 @@ struct ClientLibManagementWorkload : public TestWorkload { state Standalone metadataStr; validClientLibMetadataSample(self->uploadedMetadataJson); metadataStr = StringRef(json_spirit::write_string(json_spirit::mValue(self->uploadedMetadataJson))); - getClientLibIdFromMetadataJson(metadataStr, self->uploadedClientLibId); - wait(testExpectedError(uploadClientLibrary(cx, metadataStr, self->generatedFileName), + self->uploadedClientLibId = getClientLibIdFromMetadataJson(metadataStr); + wait(testExpectedError(uploadClientLibrary(cx, metadataStr, StringRef(self->generatedFileName)), "uploadClientLibrary wrong checksum", client_lib_invalid_binary(), &self->success)); @@ -181,11 +181,11 @@ struct ClientLibManagementWorkload : public TestWorkload { // avoid clientLibId clashes, when multiple clients try to upload the same file self->uploadedMetadataJson[CLIENTLIB_ATTR_TYPE] = format("devbuild%d", self->clientId); metadataStr = StringRef(json_spirit::write_string(json_spirit::mValue(self->uploadedMetadataJson))); - getClientLibIdFromMetadataJson(metadataStr, self->uploadedClientLibId); + self->uploadedClientLibId = getClientLibIdFromMetadataJson(metadataStr); // Test two concurrent uploads of the same library, one of the must fail and another succeed for (int i1 = 0; i1 < 2; i1++) { - Future uploadActor = uploadClientLibrary(cx, metadataStr, self->generatedFileName); + Future uploadActor = uploadClientLibrary(cx, metadataStr, StringRef(self->generatedFileName)); concurrentUploads.push_back(errorOr(uploadActor)); } @@ -214,15 +214,15 @@ struct ClientLibManagementWorkload : public TestWorkload { ACTOR static Future testClientLibDownloadNotExisting(ClientLibManagementWorkload* self, Database cx) { // Generate a random valid clientLibId - state std::string clientLibId; + state Standalone clientLibId; state std::string destFileName; json_spirit::mObject metadataJson; validClientLibMetadataSample(metadataJson); Standalone metadataStr = StringRef(json_spirit::write_string(json_spirit::mValue(metadataJson))); - getClientLibIdFromMetadataJson(metadataStr, clientLibId); + clientLibId = getClientLibIdFromMetadataJson(metadataStr); destFileName = format("clientLibDownload%d", self->clientId); - wait(testExpectedError(downloadClientLibrary(cx, clientLibId, destFileName), + wait(testExpectedError(downloadClientLibrary(cx, StringRef(clientLibId), StringRef(destFileName)), "download not existing client library", client_lib_not_found(), &self->success)); @@ -274,8 +274,8 @@ struct ClientLibManagementWorkload : public TestWorkload { wait(testUploadedClientLibInList(self, cx, filter, false, "Filter available, newer API")); filter = ClientLibFilter().filterCompatibleAPI(uploadedApiVersion).filterPlatform(uploadedPlatform); wait(testUploadedClientLibInList(self, cx, filter, true, "Filter the same API, the same platform")); - ASSERT(uploadedPlatform != CLIENTLIB_X86_64_WINDOWS); - filter = ClientLibFilter().filterAvailable().filterPlatform(CLIENTLIB_X86_64_WINDOWS); + ASSERT(uploadedPlatform != ClientLibPlatform::X86_64_WINDOWS); + filter = ClientLibFilter().filterAvailable().filterPlatform(ClientLibPlatform::X86_64_WINDOWS); wait(testUploadedClientLibInList(self, cx, filter, false, "Filter available, different platform")); filter = ClientLibFilter().filterAvailable().filterNewerPackageVersion(uploadedVersion); wait(testUploadedClientLibInList(self, cx, filter, false, "Filter available, the same version")); @@ -304,8 +304,8 @@ struct ClientLibManagementWorkload : public TestWorkload { Standalone> allLibs = wait(listClientLibraries(cx, filter)); bool found = false; for (StringRef metadataJson : allLibs) { - std::string clientLibId; - getClientLibIdFromMetadataJson(metadataJson, clientLibId); + Standalone clientLibId; + clientLibId = getClientLibIdFromMetadataJson(metadataJson); if (clientLibId == self->uploadedClientLibId) { found = true; } @@ -340,12 +340,12 @@ struct ClientLibManagementWorkload : public TestWorkload { static void validClientLibMetadataSample(json_spirit::mObject& metadataJson) { metadataJson.clear(); - metadataJson[CLIENTLIB_ATTR_PLATFORM] = getPlatformName(CLIENTLIB_X86_64_LINUX); + metadataJson[CLIENTLIB_ATTR_PLATFORM] = getPlatformName(ClientLibPlatform::X86_64_LINUX); metadataJson[CLIENTLIB_ATTR_VERSION] = "7.1.0"; metadataJson[CLIENTLIB_ATTR_GIT_HASH] = randomHexadecimalStr(40); metadataJson[CLIENTLIB_ATTR_TYPE] = "debug"; metadataJson[CLIENTLIB_ATTR_CHECKSUM] = randomHexadecimalStr(32); - metadataJson[CLIENTLIB_ATTR_STATUS] = getStatusName(CLIENTLIB_AVAILABLE); + metadataJson[CLIENTLIB_ATTR_STATUS] = getStatusName(ClientLibStatus::AVAILABLE); metadataJson[CLIENTLIB_ATTR_API_VERSION] = 710; metadataJson[CLIENTLIB_ATTR_PROTOCOL] = "fdb00b07001001"; metadataJson[CLIENTLIB_ATTR_CHECKSUM_ALG] = "md5"; @@ -360,7 +360,7 @@ struct ClientLibManagementWorkload : public TestWorkload { ASSERT(actualError.isValid()); if (expectedError.code() != actualError.code()) { TraceEvent evt(SevError, "TestErrorCodeFailed", id); - evt.detail("TestDesc", testDescr); + evt.detail("TestDescription", testDescr); evt.detail("ExpectedError", expectedError.code()); evt.error(actualError); for (auto& p : details) { diff --git a/fdbserver/workloads/workloads.actor.h b/fdbserver/workloads/workloads.actor.h index b27e138aa0..eaa0936cf2 100644 --- a/fdbserver/workloads/workloads.actor.h +++ b/fdbserver/workloads/workloads.actor.h @@ -237,14 +237,15 @@ Future quietDatabase(Database const& cx, * * In case of a failure, logs an corresponding error in the trace with the given * description and details, sets the given success flag to false (optional) - * and throws the given exception (optional). + * and throws the given exception (optional). Note that in case of a successful + * test execution, the success flag is kept unchanged. */ Future testExpectedError(Future test, const char* testDescr, - Error expectedError = Error(), - bool* successFlag = nullptr, + Optional expectedError = Optional(), + Optional successFlag = Optional(), std::map details = {}, - Error throwOnError = Error(), + Optional throwOnError = Optional(), UID id = UID()); #include "flow/unactorcompiler.h"