From 71213506ee0dd109390b01c05b4eb4fe90eb9bcc Mon Sep 17 00:00:00 2001 From: Jingyu Zhou Date: Sat, 5 Nov 2022 11:36:45 -0700 Subject: [PATCH 001/468] Fix transaction_too_old error when version vector is enabled When VV is enabled, the comparison of storage server version and read version should use the original read version, otherwise, the client may get the wrong transaction_too_old error. --- fdbserver/storageserver.actor.cpp | 40 +++++++++++++++++++++++++++---- 1 file changed, 36 insertions(+), 4 deletions(-) diff --git a/fdbserver/storageserver.actor.cpp b/fdbserver/storageserver.actor.cpp index edda4be1901..5aa75b5eee6 100644 --- a/fdbserver/storageserver.actor.cpp +++ b/fdbserver/storageserver.actor.cpp @@ -2920,8 +2920,10 @@ ACTOR Future quickGetValue(StorageServer* data, // If limit>=0, it returns the first rows in the range (sorted ascending), otherwise the last rows (sorted descending). // readRange has O(|result|) + O(log |data|) cost +// origVersion: original read version when version vector is enabled ACTOR Future readRange(StorageServer* data, Version version, + Version origVersion, KeyRange range, int limit, int* pLimitBytes, @@ -3006,8 +3008,15 @@ ACTOR Future readRange(StorageServer* data, data->counters.kvScanBytes += atStorageVersion.logicalSize(); ASSERT(atStorageVersion.size() <= limit); - if (data->storageVersion() > version) + if ((!SERVER_KNOBS->ENABLE_VERSION_VECTOR && data->storageVersion() > version) || + (SERVER_KNOBS->ENABLE_VERSION_VECTOR && data->storageVersion() > origVersion)) { + DisabledTraceEvent("SS_TTO", data->thisServerID) + .detail("StorageVersion", data->storageVersion()) + .detail("Version", version) + .detail("OrigVersion", origVersion) + .detail("Range", range); throw transaction_too_old(); + } // merge the sets in resultCache with the sets on disk, stopping at the last key from disk if there is // 'more' @@ -3099,8 +3108,15 @@ ACTOR Future readRange(StorageServer* data, data->counters.kvScanBytes += atStorageVersion.logicalSize(); ASSERT(atStorageVersion.size() <= -limit); - if (data->storageVersion() > version) + if ((!SERVER_KNOBS->ENABLE_VERSION_VECTOR && data->storageVersion() > version) || + (SERVER_KNOBS->ENABLE_VERSION_VECTOR && data->storageVersion() > origVersion)) { + DisabledTraceEvent("SS_TTO", data->thisServerID) + .detail("StorageVersion", data->storageVersion()) + .detail("Version", version) + .detail("OrigVersion", origVersion) + .detail("Range", range); throw transaction_too_old(); + } int prevSize = result.data.size(); merge(result.arena, @@ -3199,6 +3215,7 @@ ACTOR Future findKey(StorageServer* data, state GetKeyValuesReply rep = wait( readRange(data, + version, version, forward ? KeyRangeRef(sel.getKey(), range.end) : KeyRangeRef(range.begin, keyAfter(sel.getKey())), (distance + skipEqualKey) * sign, @@ -3214,6 +3231,7 @@ ACTOR Future findKey(StorageServer* data, TEST(true); // Reverse key selector returned only one result in range read maxBytes = std::numeric_limits::max(); GetKeyValuesReply rep2 = wait(readRange(data, + version, version, KeyRangeRef(range.begin, keyAfter(sel.getKey())), -2, @@ -3318,6 +3336,11 @@ ACTOR Future getKeyValuesQ(StorageServer* data, GetKeyValuesRequest req) Version commitVersion = getLatestCommitVersion(req.ssLatestCommitVersions, data->tag); state Version version = wait(waitForVersion(data, commitVersion, req.version, span.context)); + DisabledTraceEvent("VVV", data->thisServerID) + .detail("Version", version) + .detail("ReqVersion", req.version) + .detail("VV", req.ssLatestCommitVersions.toString()) + .detail("DebugID", req.debugID.present() ? req.debugID.get() : UID()); data->counters.readVersionWaitSample.addMeasurement(g_network->timer() - queueWaitEnd); state Optional tenantEntry = data->getTenantEntry(version, req.tenantInfo); @@ -3398,6 +3421,7 @@ ACTOR Future getKeyValuesQ(StorageServer* data, GetKeyValuesRequest req) state double kvReadRange = g_network->timer(); GetKeyValuesReply _r = wait(readRange(data, version, + req.version, KeyRangeRef(begin, end), req.limit, &remainingLimitBytes, @@ -4029,6 +4053,7 @@ ACTOR Future getMappedKeyValuesQ(StorageServer* data, GetMappedKeyValuesRe GetKeyValuesReply getKeyValuesReply = wait(readRange(data, version, + req.version, KeyRangeRef(begin, end), req.limit, &remainingLimitBytes, @@ -4232,8 +4257,15 @@ ACTOR Future getKeyValuesStreamQ(StorageServer* data, GetKeyValuesStreamRe !data->isTss() && !data->isSSWithTSSPair()) ? 1 : CLIENT_KNOBS->REPLY_BYTE_LIMIT; - GetKeyValuesReply _r = wait(readRange( - data, version, KeyRangeRef(begin, end), req.limit, &byteLimit, span.context, type, tenantPrefix)); + GetKeyValuesReply _r = wait(readRange(data, + version, + req.version, + KeyRangeRef(begin, end), + req.limit, + &byteLimit, + span.context, + type, + tenantPrefix)); GetKeyValuesStreamReply r(_r); if (req.debugID.present()) From 3371992e07e5197e77d157f9eb875d4a4f43f52f Mon Sep 17 00:00:00 2001 From: Jingyu Zhou Date: Sat, 5 Nov 2022 12:02:50 -0700 Subject: [PATCH 002/468] Fix assertions w.r.t. VV --- fdbserver/storageserver.actor.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/fdbserver/storageserver.actor.cpp b/fdbserver/storageserver.actor.cpp index 5aa75b5eee6..bafeee51da1 100644 --- a/fdbserver/storageserver.actor.cpp +++ b/fdbserver/storageserver.actor.cpp @@ -2978,7 +2978,8 @@ ACTOR Future readRange(StorageServer* data, while (limit > 0 && *pLimitBytes > 0 && readBegin < range.end) { ASSERT(!vCurrent || vCurrent.key() >= readBegin); - ASSERT(data->storageVersion() <= version); + ASSERT((!SERVER_KNOBS->ENABLE_VERSION_VECTOR && data->storageVersion() <= version) || + (SERVER_KNOBS->ENABLE_VERSION_VECTOR && data->storageVersion() <= origVersion)); /* Traverse the PTree further, if thare are no unconsumed resultCache items */ if (pos == resultCache.size()) { @@ -3078,7 +3079,8 @@ ACTOR Future readRange(StorageServer* data, while (limit < 0 && *pLimitBytes > 0 && readEnd > range.begin) { ASSERT(!vCurrent || vCurrent.key() < readEnd); - ASSERT(data->storageVersion() <= version); + ASSERT((!SERVER_KNOBS->ENABLE_VERSION_VECTOR && data->storageVersion() <= version) || + (SERVER_KNOBS->ENABLE_VERSION_VECTOR && data->storageVersion() <= origVersion)); /* Traverse the PTree further, if thare are no unconsumed resultCache items */ if (pos == resultCache.size()) { From c3f25a2edfbb90896b69dec5d88976132e13e66f Mon Sep 17 00:00:00 2001 From: Jingyu Zhou Date: Mon, 7 Nov 2022 14:29:21 -0800 Subject: [PATCH 003/468] Avoid using oldest version as read version for VV --- fdbserver/storageserver.actor.cpp | 44 +++++++------------ .../workloads/ConsistencyCheck.actor.cpp | 2 + 2 files changed, 19 insertions(+), 27 deletions(-) diff --git a/fdbserver/storageserver.actor.cpp b/fdbserver/storageserver.actor.cpp index bafeee51da1..91477d05d11 100644 --- a/fdbserver/storageserver.actor.cpp +++ b/fdbserver/storageserver.actor.cpp @@ -1521,7 +1521,14 @@ Future waitForVersion(StorageServer* data, Version commitVersion, Versi return transaction_too_old(); } else { if (commitVersion < data->oldestVersion.get()) { - return data->oldestVersion.get(); + if (data->version.get() < readVersion) { + // Majority of the case, try using higher version to avoid + // transaction_too_old error when oldestVersion advances. + return data->version.get(); // majority of cases + } else { + ASSERT(readVersion >= data->oldestVersion.get()); + return readVersion; + } } else if (commitVersion <= data->version.get()) { return commitVersion; } @@ -2920,10 +2927,8 @@ ACTOR Future quickGetValue(StorageServer* data, // If limit>=0, it returns the first rows in the range (sorted ascending), otherwise the last rows (sorted descending). // readRange has O(|result|) + O(log |data|) cost -// origVersion: original read version when version vector is enabled ACTOR Future readRange(StorageServer* data, Version version, - Version origVersion, KeyRange range, int limit, int* pLimitBytes, @@ -2978,8 +2983,7 @@ ACTOR Future readRange(StorageServer* data, while (limit > 0 && *pLimitBytes > 0 && readBegin < range.end) { ASSERT(!vCurrent || vCurrent.key() >= readBegin); - ASSERT((!SERVER_KNOBS->ENABLE_VERSION_VECTOR && data->storageVersion() <= version) || - (SERVER_KNOBS->ENABLE_VERSION_VECTOR && data->storageVersion() <= origVersion)); + ASSERT(data->storageVersion() <= version); /* Traverse the PTree further, if thare are no unconsumed resultCache items */ if (pos == resultCache.size()) { @@ -3009,12 +3013,11 @@ ACTOR Future readRange(StorageServer* data, data->counters.kvScanBytes += atStorageVersion.logicalSize(); ASSERT(atStorageVersion.size() <= limit); - if ((!SERVER_KNOBS->ENABLE_VERSION_VECTOR && data->storageVersion() > version) || - (SERVER_KNOBS->ENABLE_VERSION_VECTOR && data->storageVersion() > origVersion)) { + if (data->storageVersion() > version) { DisabledTraceEvent("SS_TTO", data->thisServerID) .detail("StorageVersion", data->storageVersion()) + .detail("Oldest", data->oldestVersion.get()) .detail("Version", version) - .detail("OrigVersion", origVersion) .detail("Range", range); throw transaction_too_old(); } @@ -3079,8 +3082,7 @@ ACTOR Future readRange(StorageServer* data, while (limit < 0 && *pLimitBytes > 0 && readEnd > range.begin) { ASSERT(!vCurrent || vCurrent.key() < readEnd); - ASSERT((!SERVER_KNOBS->ENABLE_VERSION_VECTOR && data->storageVersion() <= version) || - (SERVER_KNOBS->ENABLE_VERSION_VECTOR && data->storageVersion() <= origVersion)); + ASSERT(data->storageVersion() <= version); /* Traverse the PTree further, if thare are no unconsumed resultCache items */ if (pos == resultCache.size()) { @@ -3110,12 +3112,10 @@ ACTOR Future readRange(StorageServer* data, data->counters.kvScanBytes += atStorageVersion.logicalSize(); ASSERT(atStorageVersion.size() <= -limit); - if ((!SERVER_KNOBS->ENABLE_VERSION_VECTOR && data->storageVersion() > version) || - (SERVER_KNOBS->ENABLE_VERSION_VECTOR && data->storageVersion() > origVersion)) { - DisabledTraceEvent("SS_TTO", data->thisServerID) + if (data->storageVersion() > version) { + TraceEvent("SS_TTO", data->thisServerID) .detail("StorageVersion", data->storageVersion()) .detail("Version", version) - .detail("OrigVersion", origVersion) .detail("Range", range); throw transaction_too_old(); } @@ -3217,7 +3217,6 @@ ACTOR Future findKey(StorageServer* data, state GetKeyValuesReply rep = wait( readRange(data, - version, version, forward ? KeyRangeRef(sel.getKey(), range.end) : KeyRangeRef(range.begin, keyAfter(sel.getKey())), (distance + skipEqualKey) * sign, @@ -3233,7 +3232,6 @@ ACTOR Future findKey(StorageServer* data, TEST(true); // Reverse key selector returned only one result in range read maxBytes = std::numeric_limits::max(); GetKeyValuesReply rep2 = wait(readRange(data, - version, version, KeyRangeRef(range.begin, keyAfter(sel.getKey())), -2, @@ -3341,6 +3339,7 @@ ACTOR Future getKeyValuesQ(StorageServer* data, GetKeyValuesRequest req) DisabledTraceEvent("VVV", data->thisServerID) .detail("Version", version) .detail("ReqVersion", req.version) + .detail("Oldest", data->oldestVersion.get()) .detail("VV", req.ssLatestCommitVersions.toString()) .detail("DebugID", req.debugID.present() ? req.debugID.get() : UID()); data->counters.readVersionWaitSample.addMeasurement(g_network->timer() - queueWaitEnd); @@ -3423,7 +3422,6 @@ ACTOR Future getKeyValuesQ(StorageServer* data, GetKeyValuesRequest req) state double kvReadRange = g_network->timer(); GetKeyValuesReply _r = wait(readRange(data, version, - req.version, KeyRangeRef(begin, end), req.limit, &remainingLimitBytes, @@ -4055,7 +4053,6 @@ ACTOR Future getMappedKeyValuesQ(StorageServer* data, GetMappedKeyValuesRe GetKeyValuesReply getKeyValuesReply = wait(readRange(data, version, - req.version, KeyRangeRef(begin, end), req.limit, &remainingLimitBytes, @@ -4259,15 +4256,8 @@ ACTOR Future getKeyValuesStreamQ(StorageServer* data, GetKeyValuesStreamRe !data->isTss() && !data->isSSWithTSSPair()) ? 1 : CLIENT_KNOBS->REPLY_BYTE_LIMIT; - GetKeyValuesReply _r = wait(readRange(data, - version, - req.version, - KeyRangeRef(begin, end), - req.limit, - &byteLimit, - span.context, - type, - tenantPrefix)); + GetKeyValuesReply _r = wait(readRange( + data, version, KeyRangeRef(begin, end), req.limit, &byteLimit, span.context, type, tenantPrefix)); GetKeyValuesStreamReply r(_r); if (req.debugID.present()) diff --git a/fdbserver/workloads/ConsistencyCheck.actor.cpp b/fdbserver/workloads/ConsistencyCheck.actor.cpp index 3d57e9fc5b4..65a5592b450 100644 --- a/fdbserver/workloads/ConsistencyCheck.actor.cpp +++ b/fdbserver/workloads/ConsistencyCheck.actor.cpp @@ -1343,6 +1343,8 @@ struct ConsistencyCheckWorkload : TestWorkload { req.limitBytes = CLIENT_KNOBS->REPLY_BYTE_LIMIT; req.version = version; req.tags = TagSet(); + req.debugID = debugRandom()->randomUniqueID(); + DisabledTraceEvent("CCD", req.debugID.get()).detail("Version", version); // Try getting the entries in the specified range state std::vector>> keyValueFutures; From 18add2ad7fb607f6c5a705b9f0a6e6459f26bae8 Mon Sep 17 00:00:00 2001 From: Jingyu Zhou Date: Mon, 7 Nov 2022 14:49:37 -0800 Subject: [PATCH 004/468] Disable a debugging trace event --- fdbserver/storageserver.actor.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fdbserver/storageserver.actor.cpp b/fdbserver/storageserver.actor.cpp index 91477d05d11..48705d31977 100644 --- a/fdbserver/storageserver.actor.cpp +++ b/fdbserver/storageserver.actor.cpp @@ -3113,7 +3113,7 @@ ACTOR Future readRange(StorageServer* data, ASSERT(atStorageVersion.size() <= -limit); if (data->storageVersion() > version) { - TraceEvent("SS_TTO", data->thisServerID) + DisabledTraceEvent("SS_TTO", data->thisServerID) .detail("StorageVersion", data->storageVersion()) .detail("Version", version) .detail("Range", range); From 339543a8a4bff550a4e7c4ab756d9561eda3258b Mon Sep 17 00:00:00 2001 From: Dan Lambright Date: Tue, 8 Nov 2022 11:06:25 -0500 Subject: [PATCH 005/468] Cherry pick 8630 --- bindings/c/test/apitester/TesterOptions.h | 1 + bindings/c/test/apitester/fdb_c_api_tester.cpp | 12 +++++++++++- bindings/c/test/apitester/run_c_api_tests.py | 10 +++++++++- fdbclient/ClientKnobs.cpp | 1 - fdbclient/MultiVersionTransaction.actor.cpp | 11 ++++++----- fdbclient/MultiVersionTransaction.h | 2 ++ fdbclient/vexillographer/fdb.options | 2 ++ 7 files changed, 31 insertions(+), 8 deletions(-) diff --git a/bindings/c/test/apitester/TesterOptions.h b/bindings/c/test/apitester/TesterOptions.h index 0f60ae436fa..8c73f5d3e1a 100644 --- a/bindings/c/test/apitester/TesterOptions.h +++ b/bindings/c/test/apitester/TesterOptions.h @@ -42,6 +42,7 @@ class TesterOptions { int numClients; std::vector> knobs; TestSpec testSpec; + bool retainClientLibCopies = false; }; } // namespace FdbApiTester diff --git a/bindings/c/test/apitester/fdb_c_api_tester.cpp b/bindings/c/test/apitester/fdb_c_api_tester.cpp index 062ffb95f71..4ee484db9aa 100644 --- a/bindings/c/test/apitester/fdb_c_api_tester.cpp +++ b/bindings/c/test/apitester/fdb_c_api_tester.cpp @@ -45,7 +45,8 @@ enum TesterOptionId { OPT_TRACE_FORMAT, OPT_KNOB, OPT_EXTERNAL_CLIENT_LIBRARY, - OPT_TEST_FILE + OPT_TEST_FILE, + OPT_RETAIN_CLIENT_LIB_COPIES }; CSimpleOpt::SOption TesterOptionDefs[] = // @@ -61,6 +62,7 @@ CSimpleOpt::SOption TesterOptionDefs[] = // { OPT_EXTERNAL_CLIENT_LIBRARY, "--external-client-library", SO_REQ_SEP }, { OPT_TEST_FILE, "-f", SO_REQ_SEP }, { OPT_TEST_FILE, "--test-file", SO_REQ_SEP }, + { OPT_RETAIN_CLIENT_LIB_COPIES, "--retain-client-lib-copies", SO_NONE }, SO_END_OF_OPTIONS }; void printProgramUsage(const char* execName) { @@ -144,6 +146,10 @@ bool processArg(TesterOptions& options, const CSimpleOpt& args) { options.testFile = args.OptionArg(); options.testSpec = readTomlTestSpec(options.testFile); break; + + case OPT_RETAIN_CLIENT_LIB_COPIES: + options.retainClientLibCopies = true; + break; } return true; } @@ -205,6 +211,10 @@ void applyNetworkOptions(TesterOptions& options) { fdb_check(FdbApi::setOption(FDBNetworkOption::FDB_NET_OPTION_TRACE_LOG_GROUP, options.logGroup)); } + if (options.retainClientLibCopies) { + fdb_check(FdbApi::setOption(FDBNetworkOption::FDB_NET_OPTION_RETAIN_CLIENT_LIBRARY_COPIES)); + } + for (auto knob : options.knobs) { fdb_check(FdbApi::setOption(FDBNetworkOption::FDB_NET_OPTION_KNOB, fmt::format("{}={}", knob.first.c_str(), knob.second.c_str()))); diff --git a/bindings/c/test/apitester/run_c_api_tests.py b/bindings/c/test/apitester/run_c_api_tests.py index 8f79e6d8b11..39067ae4a86 100755 --- a/bindings/c/test/apitester/run_c_api_tests.py +++ b/bindings/c/test/apitester/run_c_api_tests.py @@ -54,6 +54,9 @@ def run_tester(args, test_file): if args.external_client_library is not None: cmd += ["--external-client-library", args.external_client_library] + if args.retain_client_lib_copies: + cmd += ["--retain-client-lib-copies"] + get_logger().info('\nRunning tester \'%s\'...' % ' '.join(cmd)) proc = Popen(cmd, stdout=sys.stdout, stderr=sys.stderr) timed_out = False @@ -111,7 +114,12 @@ def parse_args(argv): help='The timeout in seconds for running each individual test. (default 300)') parser.add_argument('--logging-level', type=str, default='INFO', choices=['ERROR', 'WARNING', 'INFO', 'DEBUG'], help='Specifies the level of detail in the tester output (default=\'INFO\').') - + parser.add_argument( + "--retain-client-lib-copies", + action="store_true", + default=False, + help="Retain temporary external client library copies.", + ) return parser.parse_args(argv) diff --git a/fdbclient/ClientKnobs.cpp b/fdbclient/ClientKnobs.cpp index 7fa8d890294..16c8191cee4 100644 --- a/fdbclient/ClientKnobs.cpp +++ b/fdbclient/ClientKnobs.cpp @@ -203,7 +203,6 @@ void ClientKnobs::initialize(Randomize randomize) { init( DEFAULT_AUTO_LOGS, 3 ); init( DEFAULT_COMMIT_GRV_PROXIES_RATIO, 3 ); init( DEFAULT_MAX_GRV_PROXIES, 4 ); - init( UNLINKONLOAD_FDBCLIB, true ); // if false, don't delete libfdb_c in tmp directory on client connect. init( IS_ACCEPTABLE_DELAY, 1.5 ); diff --git a/fdbclient/MultiVersionTransaction.actor.cpp b/fdbclient/MultiVersionTransaction.actor.cpp index c21273975ca..d5cbae914f8 100644 --- a/fdbclient/MultiVersionTransaction.actor.cpp +++ b/fdbclient/MultiVersionTransaction.actor.cpp @@ -2134,6 +2134,9 @@ void MultiVersionApi::setNetworkOptionInternal(FDBNetworkOptions::Option option, // multiple client threads are not supported on windows. threadCount = extractIntOption(value, 1, 1); #endif + } else if (option == FDBNetworkOptions::RETAIN_CLIENT_LIBRARY_COPIES) { + validateOption(value, false, true); + retainClientLibCopies = true; } else { forwardOption = true; } @@ -2177,10 +2180,7 @@ void MultiVersionApi::setupNetwork() { if (externalClients.count(filename) == 0) { externalClients[filename] = {}; for (const auto& tmp : copyExternalLibraryPerThread(path)) { - bool unlinkOnLoad = tmp.second; - if (!CLIENT_KNOBS->UNLINKONLOAD_FDBCLIB) { - unlinkOnLoad = false; - } + bool unlinkOnLoad = tmp.second && !retainClientLibCopies; externalClients[filename].push_back(Reference( new ClientInfo(new DLApi(tmp.first, unlinkOnLoad /*unlink on load*/), path))); } @@ -2513,7 +2513,8 @@ void MultiVersionApi::loadEnvironmentVariableNetworkOptions() { MultiVersionApi::MultiVersionApi() : callbackOnMainThread(true), localClientDisabled(false), networkStartSetup(false), networkSetup(false), - bypassMultiClientApi(false), externalClient(false), apiVersion(0), threadCount(0), envOptionsLoaded(false) {} + bypassMultiClientApi(false), externalClient(false), retainClientLibCopies(false), apiVersion(0), threadCount(0), + envOptionsLoaded(false) {} MultiVersionApi* MultiVersionApi::api = new MultiVersionApi(); diff --git a/fdbclient/MultiVersionTransaction.h b/fdbclient/MultiVersionTransaction.h index e2b0c7e027c..6e172d33513 100644 --- a/fdbclient/MultiVersionTransaction.h +++ b/fdbclient/MultiVersionTransaction.h @@ -894,6 +894,8 @@ class MultiVersionApi : public IClientApi { volatile bool networkSetup; volatile bool bypassMultiClientApi; volatile bool externalClient; + bool retainClientLibCopies; + int apiVersion; int nextThread = 0; diff --git a/fdbclient/vexillographer/fdb.options b/fdbclient/vexillographer/fdb.options index 9f2f9e52f93..bb9c5d2cff2 100644 --- a/fdbclient/vexillographer/fdb.options +++ b/fdbclient/vexillographer/fdb.options @@ -30,6 +30,8 @@ description is not currently required but encouraged. +