From c8bac58536f6604abd339788db47c9f4d545b1e2 Mon Sep 17 00:00:00 2001 From: sfc-gh-tclinkenbeard Date: Fri, 25 Mar 2022 09:10:19 -0700 Subject: [PATCH 1/7] Fix typo in SimpleKeyValueStoreInterface class name --- documentation/tutorial/tutorial.actor.cpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/documentation/tutorial/tutorial.actor.cpp b/documentation/tutorial/tutorial.actor.cpp index a42677f0ee..8f3662eb8d 100644 --- a/documentation/tutorial/tutorial.actor.cpp +++ b/documentation/tutorial/tutorial.actor.cpp @@ -238,7 +238,7 @@ ACTOR Future echoClient() { return Void(); } -struct SimpleKeyValueStoreInteface { +struct SimpleKeyValueStoreInterface { constexpr static FileIdentifier file_identifier = 8226647; RequestStream connect; RequestStream get; @@ -253,7 +253,7 @@ struct SimpleKeyValueStoreInteface { struct GetKVInterface { constexpr static FileIdentifier file_identifier = 8062308; - ReplyPromise reply; + ReplyPromise reply; template void serialize(Ar& ar) { @@ -297,7 +297,7 @@ struct ClearRequest { }; ACTOR Future kvStoreServer() { - state SimpleKeyValueStoreInteface inf; + state SimpleKeyValueStoreInterface inf; state std::map store; inf.connect.makeWellKnownEndpoint(WLTOKEN_SIMPLE_KV_SERVER, TaskPriority::DefaultEndpoint); loop { @@ -333,17 +333,17 @@ ACTOR Future kvStoreServer() { } } -ACTOR Future connect() { +ACTOR Future connect() { std::cout << format("%llu: Connect...\n", uint64_t(g_network->now())); - SimpleKeyValueStoreInteface c; + SimpleKeyValueStoreInterface c; c.connect = RequestStream(Endpoint::wellKnown({ serverAddress }, WLTOKEN_SIMPLE_KV_SERVER)); - SimpleKeyValueStoreInteface result = wait(c.connect.getReply(GetKVInterface())); + SimpleKeyValueStoreInterface result = wait(c.connect.getReply(GetKVInterface())); std::cout << format("%llu: done..\n", uint64_t(g_network->now())); return result; } ACTOR Future kvSimpleClient() { - state SimpleKeyValueStoreInteface server = wait(connect()); + state SimpleKeyValueStoreInterface server = wait(connect()); std::cout << format("Set %s -> %s\n", "foo", "bar"); SetRequest setRequest; setRequest.key = "foo"; @@ -356,7 +356,7 @@ ACTOR Future kvSimpleClient() { return Void(); } -ACTOR Future kvClient(SimpleKeyValueStoreInteface server, std::shared_ptr ops) { +ACTOR Future kvClient(SimpleKeyValueStoreInterface server, std::shared_ptr ops) { state Future timeout = delay(20); state int rangeSize = 2 << 12; loop { @@ -397,7 +397,7 @@ ACTOR Future throughputMeasurement(std::shared_ptr operations) { } ACTOR Future multipleClients() { - SimpleKeyValueStoreInteface server = wait(connect()); + SimpleKeyValueStoreInterface server = wait(connect()); auto ops = std::make_shared(0); std::vector> clients(100); for (auto& f : clients) { From e8a38bb04834d1565ea4132122cedb775e7057eb Mon Sep 17 00:00:00 2001 From: "A.J. Beamon" Date: Fri, 25 Mar 2022 14:24:25 -0700 Subject: [PATCH 2/7] Fix bug in tenant map range reads when the end key is outside of the tenant map range. --- bindings/c/test/unit/unit_tests.cpp | 33 +++++++++++++++++++++++++++++ fdbclient/SpecialKeySpace.actor.cpp | 19 +++++++++++------ 2 files changed, 46 insertions(+), 6 deletions(-) diff --git a/bindings/c/test/unit/unit_tests.cpp b/bindings/c/test/unit/unit_tests.cpp index 9cb2b3ae5a..b5c508736f 100644 --- a/bindings/c/test/unit/unit_tests.cpp +++ b/bindings/c/test/unit/unit_tests.cpp @@ -20,6 +20,7 @@ // Unit tests for the FoundationDB C API. +#include "fdb_c_options.g.h" #define FDB_API_VERSION 710 #include #include @@ -2430,6 +2431,38 @@ TEST_CASE("Tenant create, access, and delete") { break; } + while (1) { + StringRef begin = "\xff\xff/management/tenant_map/"_sr; + StringRef end = "\xff\xff/management/tenant_map0"_sr; + + fdb_check(tr.set_option(FDB_TR_OPTION_SPECIAL_KEY_SPACE_ENABLE_WRITES, nullptr, 0)); + fdb::KeyValueArrayFuture f = tr.get_range(FDB_KEYSEL_FIRST_GREATER_OR_EQUAL(begin.data(), begin.size()), + FDB_KEYSEL_FIRST_GREATER_OR_EQUAL(end.data(), end.size()), + /* limit */ 0, + /* target_bytes */ 0, + /* FDBStreamingMode */ FDB_STREAMING_MODE_WANT_ALL, + /* iteration */ 0, + /* snapshot */ false, + /* reverse */ 0); + + fdb_error_t err = wait_future(f); + if (err) { + fdb::EmptyFuture f2 = tr.on_error(err); + fdb_check(wait_future(f2)); + continue; + } + + FDBKeyValue const* outKv; + int outCount; + int outMore; + fdb_check(f1.get(&outKv, &outCount, &outMore)); + CHECK(outCount == 1); + CHECK(StringRef(outKv->key, outKv->key_length) == StringRef(tenantName).withPrefix(tenantMapKeys.begin)); + + tr.reset(); + break; + } + fdb::Tenant tenant(db, reinterpret_cast(tenantName.c_str()), tenantName.size()); fdb::Transaction tr2(tenant); diff --git a/fdbclient/SpecialKeySpace.actor.cpp b/fdbclient/SpecialKeySpace.actor.cpp index 21797cbd67..fd18e94f20 100644 --- a/fdbclient/SpecialKeySpace.actor.cpp +++ b/fdbclient/SpecialKeySpace.actor.cpp @@ -2704,16 +2704,23 @@ Future> FailedLocalitiesRangeImpl::commit(ReadYourWritesTr } ACTOR Future getTenantList(ReadYourWritesTransaction* ryw, KeyRangeRef kr, GetRangeLimits limitsHint) { - KeyRangeRef tenantRange = - kr.removePrefix(SpecialKeySpace::getModuleRange(SpecialKeySpace::MODULE::MANAGEMENT).begin) - .removePrefix(TenantMapRangeImpl::submoduleRange.begin); state KeyRef managementPrefix = kr.begin.substr(0, SpecialKeySpace::getModuleRange(SpecialKeySpace::MODULE::MANAGEMENT).begin.size() + TenantMapRangeImpl::submoduleRange.begin.size()); - std::map tenants = wait(ManagementAPI::listTenantsTransaction( - &ryw->getTransaction(), tenantRange.begin, tenantRange.end, limitsHint.rows)); + kr = kr.removePrefix(SpecialKeySpace::getModuleRange(SpecialKeySpace::MODULE::MANAGEMENT).begin); + TenantNameRef beginTenant = kr.begin.removePrefix(TenantMapRangeImpl::submoduleRange.begin); + + TenantNameRef endTenant = kr.end; + if (endTenant.startsWith(TenantMapRangeImpl::submoduleRange.begin)) { + endTenant = endTenant.removePrefix(TenantMapRangeImpl::submoduleRange.begin); + } else { + endTenant = "\xff"_sr; + } + + std::map tenants = + wait(ManagementAPI::listTenantsTransaction(&ryw->getTransaction(), beginTenant, endTenant, limitsHint.rows)); RangeResult results; for (auto tenant : tenants) { @@ -2783,7 +2790,7 @@ Future> TenantMapRangeImpl::commit(ReadYourWritesTransacti TenantNameRef endTenant = range.end().removePrefix( SpecialKeySpace::getModuleRange(SpecialKeySpace::MODULE::MANAGEMENT).begin); if (endTenant.startsWith(submoduleRange.begin)) { - endTenant = endTenant.removePrefix(submoduleRange.end); + endTenant = endTenant.removePrefix(submoduleRange.begin); } else { endTenant = "\xff"_sr; } From 2475c4e2d674c1333e3dd0c5640be81583598121 Mon Sep 17 00:00:00 2001 From: "A.J. Beamon" Date: Fri, 25 Mar 2022 15:56:49 -0700 Subject: [PATCH 3/7] Fix various non-compiling code errors. --- bindings/c/test/unit/unit_tests.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/bindings/c/test/unit/unit_tests.cpp b/bindings/c/test/unit/unit_tests.cpp index b5c508736f..b9bf312a6b 100644 --- a/bindings/c/test/unit/unit_tests.cpp +++ b/bindings/c/test/unit/unit_tests.cpp @@ -2436,8 +2436,8 @@ TEST_CASE("Tenant create, access, and delete") { StringRef end = "\xff\xff/management/tenant_map0"_sr; fdb_check(tr.set_option(FDB_TR_OPTION_SPECIAL_KEY_SPACE_ENABLE_WRITES, nullptr, 0)); - fdb::KeyValueArrayFuture f = tr.get_range(FDB_KEYSEL_FIRST_GREATER_OR_EQUAL(begin.data(), begin.size()), - FDB_KEYSEL_FIRST_GREATER_OR_EQUAL(end.data(), end.size()), + fdb::KeyValueArrayFuture f = tr.get_range(FDB_KEYSEL_FIRST_GREATER_OR_EQUAL(begin.begin(), begin.size()), + FDB_KEYSEL_FIRST_GREATER_OR_EQUAL(end.begin(), end.size()), /* limit */ 0, /* target_bytes */ 0, /* FDBStreamingMode */ FDB_STREAMING_MODE_WANT_ALL, @@ -2455,9 +2455,9 @@ TEST_CASE("Tenant create, access, and delete") { FDBKeyValue const* outKv; int outCount; int outMore; - fdb_check(f1.get(&outKv, &outCount, &outMore)); + fdb_check(f.get(&outKv, &outCount, &outMore)); CHECK(outCount == 1); - CHECK(StringRef(outKv->key, outKv->key_length) == StringRef(tenantName).withPrefix(tenantMapKeys.begin)); + CHECK(StringRef(outKv->key, outKv->key_length) == StringRef(tenantName).withPrefix(begin)); tr.reset(); break; From d58777351bd7145546080c2af0e44bae06030004 Mon Sep 17 00:00:00 2001 From: Andrew Noyes Date: Mon, 28 Mar 2022 12:36:52 -0700 Subject: [PATCH 4/7] Enforce comment with static assert (#6699) --- fdbclient/WellKnownEndpoints.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/fdbclient/WellKnownEndpoints.h b/fdbclient/WellKnownEndpoints.h index 5db3f34cea..bed5c34935 100644 --- a/fdbclient/WellKnownEndpoints.h +++ b/fdbclient/WellKnownEndpoints.h @@ -52,4 +52,7 @@ enum WellKnownEndpoints { WLTOKEN_RESERVED_COUNT // 23 }; +static_assert(WLTOKEN_PROTOCOL_INFO == + 10); // Enforce that the value of this endpoint does not change per comment above. + #endif From fd5734c39ea10f09558f61df7a3e3e93ea9bc37b Mon Sep 17 00:00:00 2001 From: Chaoguang Lin Date: Mon, 28 Mar 2022 12:19:01 -0700 Subject: [PATCH 5/7] Fix the case where we access the memory after cleaning --- fdbclient/NativeAPI.actor.cpp | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/fdbclient/NativeAPI.actor.cpp b/fdbclient/NativeAPI.actor.cpp index ac459e27c5..4b21429737 100644 --- a/fdbclient/NativeAPI.actor.cpp +++ b/fdbclient/NativeAPI.actor.cpp @@ -8707,11 +8707,24 @@ ACTOR Future singleChangeFeedStreamInternal(KeyRange range, results->lastReturnedVersion.set(feedReply.mutations.back().version); } - if (refresh.canBeSet() && !atLatest && feedReply.atLatestVersion) { + if (!refresh.canBeSet()) { + try { + // refresh is set if and only if this actor is cancelled + wait(Future(Void())); + // Catch any unexpected behavior if the above contract is broken + ASSERT(false); + } catch (Error& e) { + ASSERT(e.code() == error_code_actor_cancelled); + throw; + } + } + + if (!atLatest && feedReply.atLatestVersion) { atLatest = true; results->notAtLatest.set(0); } - if (refresh.canBeSet() && feedReply.minStreamVersion > results->storageData[0]->version.get()) { + + if (feedReply.minStreamVersion > results->storageData[0]->version.get()) { results->storageData[0]->version.set(feedReply.minStreamVersion); } } From 721c64b698a1b68138d8b4380db50620281e24d7 Mon Sep 17 00:00:00 2001 From: Josh Slocum Date: Thu, 24 Mar 2022 13:03:40 -0500 Subject: [PATCH 6/7] Adding Blob Granule Client C Unit Tests --- bindings/c/foundationdb/fdb_c.h | 4 +- bindings/c/test/unit/unit_tests.cpp | 146 ++++++++++++++++++++++++++++ fdbclient/ThreadSafeTransaction.cpp | 3 - 3 files changed, 148 insertions(+), 5 deletions(-) diff --git a/bindings/c/foundationdb/fdb_c.h b/bindings/c/foundationdb/fdb_c.h index e3c805a056..8f5d6840fa 100644 --- a/bindings/c/foundationdb/fdb_c.h +++ b/bindings/c/foundationdb/fdb_c.h @@ -449,7 +449,7 @@ DLLEXPORT WARN_UNUSED_RESULT FDBFuture* fdb_transaction_get_range_split_points(F int end_key_name_length, int64_t chunk_size); -DLLEXPORT WARN_UNUSED_RESULT FDBFuture* fdb_transaction_get_blob_granule_ranges(FDBTransaction* db, +DLLEXPORT WARN_UNUSED_RESULT FDBFuture* fdb_transaction_get_blob_granule_ranges(FDBTransaction* tr, uint8_t const* begin_key_name, int begin_key_name_length, uint8_t const* end_key_name, @@ -457,7 +457,7 @@ DLLEXPORT WARN_UNUSED_RESULT FDBFuture* fdb_transaction_get_blob_granule_ranges( /* LatestVersion (-2) for readVersion means get read version from transaction Separated out as optional because BG reads can support longer-lived reads than normal FDB transactions */ -DLLEXPORT WARN_UNUSED_RESULT FDBResult* fdb_transaction_read_blob_granules(FDBTransaction* db, +DLLEXPORT WARN_UNUSED_RESULT FDBResult* fdb_transaction_read_blob_granules(FDBTransaction* tr, uint8_t const* begin_key_name, int begin_key_name_length, uint8_t const* end_key_name, diff --git a/bindings/c/test/unit/unit_tests.cpp b/bindings/c/test/unit/unit_tests.cpp index b9bf312a6b..78cb2ee2e9 100644 --- a/bindings/c/test/unit/unit_tests.cpp +++ b/bindings/c/test/unit/unit_tests.cpp @@ -2538,6 +2538,152 @@ TEST_CASE("Tenant create, access, and delete") { } } +int64_t granule_start_load_fail(const char* filename, + int filenameLength, + int64_t offset, + int64_t length, + int64_t fullFileLength, + void* userContext) { + CHECK(false); + return -1; +} + +uint8_t* granule_get_load_fail(int64_t loadId, void* userContext) { + CHECK(false); + return nullptr; +} + +void granule_free_load_fail(int64_t loadId, void* userContext) { + CHECK(false); +} + +TEST_CASE("Blob Granule Functions") { + auto confValue = + get_value("\xff/conf/blob_granules_enabled", /* snapshot */ false, { FDB_TR_OPTION_READ_SYSTEM_KEYS }); + if (!confValue.has_value() || confValue.value() != "1") { + return; + } + + // write some data + + insert_data(db, create_data({ { "bg1", "a" }, { "bg2", "b" }, { "bg3", "c" } })); + + // because wiring up files is non-trivial, just test the calls complete with the expected no_materialize error + FDBReadBlobGranuleContext granuleContext; + granuleContext.userContext = nullptr; + granuleContext.start_load_f = &granule_start_load_fail; + granuleContext.get_load_f = &granule_get_load_fail; + granuleContext.free_load_f = &granule_free_load_fail; + granuleContext.debugNoMaterialize = true; + granuleContext.granuleParallelism = 1; + + // dummy values + FDBKeyValue const* out_kv; + int out_count; + int out_more; + + fdb::Transaction tr(db); + int64_t originalReadVersion = -1; + + // test no materialize gets error but completes, save read version + while (1) { + fdb_check(tr.set_option(FDB_TR_OPTION_READ_YOUR_WRITES_DISABLE, nullptr, 0)); + // -2 is latest version + fdb::KeyValueArrayResult r = tr.read_blob_granules(key("bg"), key("bh"), 0, -2, granuleContext); + fdb_error_t err = r.get(&out_kv, &out_count, &out_more); + if (err && err != 2037 /* blob_granule_not_materialized */) { + fdb::EmptyFuture f2 = tr.on_error(err); + fdb_check(wait_future(f2)); + continue; + } + + CHECK(err == 2037 /* blob_granule_not_materialized */); + + // If read done, save read version. Should have already used read version so this shouldn't error + fdb::Int64Future grvFuture = tr.get_read_version(); + fdb_error_t grvErr = wait_future(grvFuture); + CHECK(!grvErr); + CHECK(!grvFuture.get(&originalReadVersion)); + + CHECK(originalReadVersion > 0); + + tr.reset(); + break; + } + + // test with begin version > 0 + while (1) { + fdb_check(tr.set_option(FDB_TR_OPTION_READ_YOUR_WRITES_DISABLE, nullptr, 0)); + // -2 is latest version, read version should be >= originalReadVersion + fdb::KeyValueArrayResult r = + tr.read_blob_granules(key("bg"), key("bh"), originalReadVersion, -2, granuleContext); + fdb_error_t err = r.get(&out_kv, &out_count, &out_more); + ; + if (err && err != 2037 /* blob_granule_not_materialized */) { + fdb::EmptyFuture f2 = tr.on_error(err); + fdb_check(wait_future(f2)); + continue; + } + + CHECK(err == 2037 /* blob_granule_not_materialized */); + + tr.reset(); + break; + } + + // test with prior read version completes after delay larger than normal MVC window + // TODO: should we not do this? + std::this_thread::sleep_for(std::chrono::milliseconds(6000)); + while (1) { + fdb_check(tr.set_option(FDB_TR_OPTION_READ_YOUR_WRITES_DISABLE, nullptr, 0)); + fdb::KeyValueArrayResult r = + tr.read_blob_granules(key("bg"), key("bh"), 0, originalReadVersion, granuleContext); + fdb_error_t err = r.get(&out_kv, &out_count, &out_more); + if (err && err != 2037 /* blob_granule_not_materialized */) { + fdb::EmptyFuture f2 = tr.on_error(err); + fdb_check(wait_future(f2)); + continue; + } + + CHECK(err == 2037 /* blob_granule_not_materialized */); + + tr.reset(); + break; + } + + // test ranges + + while (1) { + fdb::KeyRangeArrayFuture f = tr.get_blob_granule_ranges(key("bg"), key("bh")); + fdb_error_t err = wait_future(f); + if (err) { + fdb::EmptyFuture f2 = tr.on_error(err); + fdb_check(wait_future(f2)); + continue; + } + + const FDBKeyRange* out_kr; + int out_count; + fdb_check(f.get(&out_kr, &out_count)); + + CHECK(out_count >= 1); + // check key ranges are in order + for (int i = 0; i < out_count; i++) { + // key range start < end + CHECK(std::string((const char*)out_kr[i].begin_key, out_kr[i].begin_key_length) < + std::string((const char*)out_kr[i].end_key, out_kr[i].end_key_length)); + } + // Ranges themselves are sorted + for (int i = 0; i < out_count - 1; i++) { + CHECK(std::string((const char*)out_kr[i].end_key, out_kr[i].end_key_length) <= + std::string((const char*)out_kr[i + 1].begin_key, out_kr[i + 1].begin_key_length)); + } + + tr.reset(); + break; + } +} + int main(int argc, char** argv) { if (argc < 3) { std::cout << "Unit tests for the FoundationDB C API.\n" diff --git a/fdbclient/ThreadSafeTransaction.cpp b/fdbclient/ThreadSafeTransaction.cpp index 146951b313..0840456459 100644 --- a/fdbclient/ThreadSafeTransaction.cpp +++ b/fdbclient/ThreadSafeTransaction.cpp @@ -319,9 +319,6 @@ ThreadResult ThreadSafeTransaction::readBlobGranules(const KeyRange Version beginVersion, Optional readVersion, ReadBlobGranuleContext granule_context) { - // In V1 of api this is required, field is just for forward compatibility - ASSERT(beginVersion == 0); - // FIXME: prevent from calling this from another main thread! ISingleThreadTransaction* tr = this->tr; From bbfaddc963d74297e1d642bd2a251487befb9452 Mon Sep 17 00:00:00 2001 From: Andrew Noyes Date: Mon, 28 Mar 2022 14:06:51 -0700 Subject: [PATCH 7/7] Don't instrument fdbmonitor with thread sanitizer (#6698) We don't compile fdbmonitor with thread sanitizer instrumentation, since this appears to change its behavior (it no longer seems to restart killed processes). fdbmonitor is single-threaded anyway. --- fdbmonitor/CMakeLists.txt | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/fdbmonitor/CMakeLists.txt b/fdbmonitor/CMakeLists.txt index 5d624e8191..622b0ec594 100644 --- a/fdbmonitor/CMakeLists.txt +++ b/fdbmonitor/CMakeLists.txt @@ -10,6 +10,16 @@ endif() # as soon as we get rid of the old build system target_link_libraries(fdbmonitor PUBLIC Threads::Threads) +# We don't compile fdbmonitor with thread sanitizer instrumentation, since this +# appears to change its behavior (it no longer seems to restart killed +# processes). fdbmonitor is single-threaded anyway. +get_target_property(fdbmonitor_options fdbmonitor COMPILE_OPTIONS) +list(REMOVE_ITEM fdbmonitor_options "-fsanitize=thread") +set_property(TARGET fdbmonitor PROPERTY COMPILE_OPTIONS ${target_options}) +get_target_property(fdbmonitor_options fdbmonitor LINK_OPTIONS) +list(REMOVE_ITEM fdbmonitor_options "-fsanitize=thread") +set_property(TARGET fdbmonitor PROPERTY LINK_OPTIONS ${target_options}) + if(GENERATE_DEBUG_PACKAGES) fdb_install(TARGETS fdbmonitor DESTINATION fdbmonitor COMPONENT server) else()