From 97925de9d46e95b50a425a53e70479fb81950a3c Mon Sep 17 00:00:00 2001 From: Chaoguang Lin Date: Fri, 5 Feb 2021 14:14:38 -0800 Subject: [PATCH] Update fdb_database_create_snapshot to take a UID as input --- bindings/c/fdb_c.cpp | 12 +++++++--- bindings/c/foundationdb/fdb_c.h | 6 +++-- bindings/c/test/unit/fdb_api.cpp | 8 +++++-- bindings/c/test/unit/fdb_api.hpp | 5 +++- bindings/c/test/unit/unit_tests.cpp | 25 +++++++++++--------- bindings/flow/fdb_flow.actor.cpp | 25 +++++++++++++------- bindings/flow/fdb_flow.h | 4 +++- fdbclient/DatabaseContext.h | 4 ++-- fdbclient/IClientApi.h | 4 +++- fdbclient/MultiVersionTransaction.actor.cpp | 26 ++++++++++++++------- fdbclient/MultiVersionTransaction.h | 20 ++++++++++------ fdbclient/NativeAPI.actor.cpp | 23 ++++++++++-------- fdbclient/ThreadSafeTransaction.cpp | 13 +++++++---- fdbclient/ThreadSafeTransaction.h | 6 +++-- flow/error_definitions.h | 1 + 15 files changed, 118 insertions(+), 64 deletions(-) diff --git a/bindings/c/fdb_c.cpp b/bindings/c/fdb_c.cpp index 23668a3a5b..515f55430e 100644 --- a/bindings/c/fdb_c.cpp +++ b/bindings/c/fdb_c.cpp @@ -398,9 +398,15 @@ extern "C" DLLEXPORT FDBFuture* fdb_database_force_recovery_with_data_loss(FDBDa return (FDBFuture*)(DB(db)->forceRecoveryWithDataLoss(StringRef(dcid, dcid_length)).extractPtr()); } -extern "C" DLLEXPORT FDBFuture* fdb_database_create_snapshot(FDBDatabase* db, uint8_t const* snap_command, - int snap_command_length) { - return (FDBFuture*)(DB(db)->createSnapshot(StringRef(snap_command, snap_command_length)).extractPtr()); +extern "C" DLLEXPORT FDBFuture * +fdb_database_create_snapshot(FDBDatabase *db, uint8_t const *uid, + int uid_length, uint8_t const *snap_command, + int snap_command_length) { + return (FDBFuture *)(DB(db) + ->createSnapshot( + StringRef(uid, uid_length), + StringRef(snap_command, snap_command_length)) + .extractPtr()); } extern "C" DLLEXPORT diff --git a/bindings/c/foundationdb/fdb_c.h b/bindings/c/foundationdb/fdb_c.h index 1b9f432cb6..9530f1e21d 100644 --- a/bindings/c/foundationdb/fdb_c.h +++ b/bindings/c/foundationdb/fdb_c.h @@ -185,8 +185,10 @@ extern "C" { DLLEXPORT WARN_UNUSED_RESULT FDBFuture* fdb_database_force_recovery_with_data_loss( FDBDatabase* db, uint8_t const* dcid, int dcid_length); - DLLEXPORT WARN_UNUSED_RESULT FDBFuture* - fdb_database_create_snapshot( FDBDatabase* db, uint8_t const* snap_command, int snap_command_length); + DLLEXPORT WARN_UNUSED_RESULT FDBFuture * + fdb_database_create_snapshot(FDBDatabase *db, uint8_t const *uid, + int uid_length, uint8_t const *snap_command, + int snap_command_length); DLLEXPORT void fdb_transaction_destroy( FDBTransaction* tr); diff --git a/bindings/c/test/unit/fdb_api.cpp b/bindings/c/test/unit/fdb_api.cpp index 2ce6579a53..fa00ba1516 100644 --- a/bindings/c/test/unit/fdb_api.cpp +++ b/bindings/c/test/unit/fdb_api.cpp @@ -102,8 +102,12 @@ EmptyFuture Database::force_recovery_with_data_loss(FDBDatabase *db, const uint8 return EmptyFuture(fdb_database_force_recovery_with_data_loss(db, dcid, dcid_length)); } -EmptyFuture Database::create_snapshot(FDBDatabase* db, const uint8_t* snap_command, int snap_command_length) { - return EmptyFuture(fdb_database_create_snapshot(db, snap_command, snap_command_length)); +EmptyFuture Database::create_snapshot(FDBDatabase *db, const uint8_t *uid, + int uid_length, + const uint8_t *snap_command, + int snap_command_length) { + return EmptyFuture(fdb_database_create_snapshot( + db, uid, uid_length, snap_command, snap_command_length)); } // Transaction diff --git a/bindings/c/test/unit/fdb_api.hpp b/bindings/c/test/unit/fdb_api.hpp index dc22134e70..a70eb1a8d0 100644 --- a/bindings/c/test/unit/fdb_api.hpp +++ b/bindings/c/test/unit/fdb_api.hpp @@ -154,7 +154,10 @@ public: static Int64Future reboot_worker(FDBDatabase* db, const uint8_t* address, int address_length, fdb_bool_t check, int duration); static EmptyFuture force_recovery_with_data_loss(FDBDatabase* db, const uint8_t* dcid, int dcid_length); - static EmptyFuture create_snapshot(FDBDatabase* db, const uint8_t* snap_command, int snap_command_length); + static EmptyFuture create_snapshot(FDBDatabase *db, const uint8_t *uid, + int uid_length, + const uint8_t *snap_command, + int snap_command_length); }; // Wrapper around FDBTransaction, providing the same set of calls as the C API. diff --git a/bindings/c/test/unit/unit_tests.cpp b/bindings/c/test/unit/unit_tests.cpp index 2e0724479f..03ab4d2d8f 100644 --- a/bindings/c/test/unit/unit_tests.cpp +++ b/bindings/c/test/unit/unit_tests.cpp @@ -2048,17 +2048,20 @@ TEST_CASE("fdb_database_force_recovery_with_data_loss") { TEST_CASE("fdb_database_create_snapshot") { std::string snapshot_command = "test"; - while (1) { - fdb::EmptyFuture f = - fdb::Database::create_snapshot(db, (const uint8_t*)snapshot_command.c_str(), snapshot_command.length()); - fdb_error_t err = wait_future(f); - if (err == 2505) { // expected error code - break; - } else { - // Otherwise, something went wrong. - CHECK(false); - } - } + std::string uid = "invalid_uid"; + while (1) { + fdb::EmptyFuture f = fdb::Database::create_snapshot( + db, (const uint8_t *)uid.c_str(), uid.length(), + (const uint8_t *)snapshot_command.c_str(), + snapshot_command.length()); + fdb_error_t err = wait_future(f); + if (err == 2509) { // expected error code + break; + } else { + // Otherwise, something went wrong. + CHECK(false); + } + } } TEST_CASE("fdb_error_predicate") { diff --git a/bindings/flow/fdb_flow.actor.cpp b/bindings/flow/fdb_flow.actor.cpp index c0e83e6889..8bf2dc5ad5 100644 --- a/bindings/flow/fdb_flow.actor.cpp +++ b/bindings/flow/fdb_flow.actor.cpp @@ -105,9 +105,11 @@ namespace FDB { void setDatabaseOption(FDBDatabaseOption option, Optional value = Optional()) override; Future rebootWorker(const StringRef& address, bool check = false, int duration = 0) override; Future forceRecoveryWithDataLoss(const StringRef& dcid) override; - Future createSnapshot(const StringRef& snap_command) override; + Future + createSnapshot(const StringRef &uid, + const StringRef &snap_command) override; - private: + private: FDBDatabase* db; explicit DatabaseImpl(FDBDatabase* db) : db(db) {} @@ -306,13 +308,18 @@ namespace FDB { }); } - Future DatabaseImpl::createSnapshot(const StringRef& snap_command) { - return backToFuture(fdb_database_create_snapshot(db, snap_command.begin(), snap_command.size()), - [](Reference f) { - throw_on_error(fdb_future_get_error(f->f)); - return Void(); - }); - } + Future + DatabaseImpl::createSnapshot(const StringRef &uid, + const StringRef &snap_command) { + return backToFuture( + fdb_database_create_snapshot(db, uid.begin(), uid.size(), + snap_command.begin(), + snap_command.size()), + [](Reference f) { + throw_on_error(fdb_future_get_error(f->f)); + return Void(); + }); + } TransactionImpl::TransactionImpl(FDBDatabase* db) { throw_on_error(fdb_database_create_transaction(db, &tr)); diff --git a/bindings/flow/fdb_flow.h b/bindings/flow/fdb_flow.h index a70f6faafa..828508c822 100644 --- a/bindings/flow/fdb_flow.h +++ b/bindings/flow/fdb_flow.h @@ -127,7 +127,9 @@ namespace FDB { virtual void setDatabaseOption(FDBDatabaseOption option, Optional value = Optional()) = 0; virtual Future rebootWorker(const StringRef& address, bool check = false, int duration = 0) = 0; virtual Future forceRecoveryWithDataLoss(const StringRef& dcid) = 0; - virtual Future createSnapshot(const StringRef& snap_command) = 0; + virtual Future + createSnapshot(const StringRef &uid, + const StringRef &snap_command) = 0; }; class API { diff --git a/fdbclient/DatabaseContext.h b/fdbclient/DatabaseContext.h index 81ea289c7d..859ba9f341 100644 --- a/fdbclient/DatabaseContext.h +++ b/fdbclient/DatabaseContext.h @@ -212,9 +212,9 @@ public: // Management API, force the database to recover into DCID, causing the database to lose the most recently committed mutations Future forceRecoveryWithDataLoss(StringRef dcId); // Management API, create snapshot - Future createSnapshot(StringRef snapshot_command); + Future createSnapshot(StringRef uid, StringRef snapshot_command); -//private: + // private: explicit DatabaseContext( Reference>> connectionFile, Reference> clientDBInfo, Future clientInfoMonitor, TaskPriority taskID, LocalityData const& clientLocality, bool enableLocalityLoadBalance, bool lockAware, bool internal = true, int apiVersion = Database::API_VERSION_LATEST, bool switchable = false ); diff --git a/fdbclient/IClientApi.h b/fdbclient/IClientApi.h index 42b75000cf..7b0cc28441 100644 --- a/fdbclient/IClientApi.h +++ b/fdbclient/IClientApi.h @@ -91,7 +91,9 @@ public: // Management API, force the database to recover into DCID, causing the database to lose the most recently committed mutations virtual ThreadFuture forceRecoveryWithDataLoss(const StringRef& dcid) = 0; // Management API, create snapshot - virtual ThreadFuture createSnapshot(const StringRef& snapshot_command) = 0; + virtual ThreadFuture + createSnapshot(const StringRef &uid, + const StringRef &snapshot_command) = 0; }; class IClientApi { diff --git a/fdbclient/MultiVersionTransaction.actor.cpp b/fdbclient/MultiVersionTransaction.actor.cpp index b4fe05526d..fac77fa465 100644 --- a/fdbclient/MultiVersionTransaction.actor.cpp +++ b/fdbclient/MultiVersionTransaction.actor.cpp @@ -311,13 +311,18 @@ ThreadFuture DLDatabase::forceRecoveryWithDataLoss(const StringRef &dcid) }); } -ThreadFuture DLDatabase::createSnapshot(const StringRef& snapshot_command) { - if (!api->databaseCreateSnapshot) { - return unsupported_operation(); - } +ThreadFuture +DLDatabase::createSnapshot(const StringRef &uid, + const StringRef &snapshot_command) { + if (!api->databaseCreateSnapshot) { + return unsupported_operation(); + } - FdbCApi::FDBFuture* f = api->databaseCreateSnapshot(db, snapshot_command.begin(), snapshot_command.size()); - return toThreadFuture(api, f, [](FdbCApi::FDBFuture* f, FdbCApi* api) { return Void(); }); + FdbCApi::FDBFuture *f = api->databaseCreateSnapshot( + db, uid.begin(), uid.size(), snapshot_command.begin(), + snapshot_command.size()); + return toThreadFuture( + api, f, [](FdbCApi::FDBFuture *f, FdbCApi *api) { return Void(); }); } // DLApi @@ -832,9 +837,12 @@ ThreadFuture MultiVersionDatabase::forceRecoveryWithDataLoss(const StringR return abortableFuture(f, dbState->dbVar->get().onChange); } -ThreadFuture MultiVersionDatabase::createSnapshot(const StringRef& snapshot_command) { - auto f = dbState->db ? dbState->db->createSnapshot(snapshot_command) : ThreadFuture(Never()); - return abortableFuture(f, dbState->dbVar->get().onChange); +ThreadFuture +MultiVersionDatabase::createSnapshot(const StringRef &uid, + const StringRef &snapshot_command) { + auto f = dbState->db ? dbState->db->createSnapshot(uid, snapshot_command) + : ThreadFuture(Never()); + return abortableFuture(f, dbState->dbVar->get().onChange); } void MultiVersionDatabase::Connector::connect() { diff --git a/fdbclient/MultiVersionTransaction.h b/fdbclient/MultiVersionTransaction.h index 73ce237125..35518842f8 100644 --- a/fdbclient/MultiVersionTransaction.h +++ b/fdbclient/MultiVersionTransaction.h @@ -68,10 +68,12 @@ struct FdbCApi : public ThreadSafeReferenceCounted { void (*databaseDestroy)(FDBDatabase *database); FDBFuture* (*databaseRebootWorker)(FDBDatabase *database, uint8_t const *address, int addressLength, fdb_bool_t check, int duration); FDBFuture* (*databaseForceRecoveryWithDataLoss)(FDBDatabase *database, uint8_t const *dcid, int dcidLength); - FDBFuture* (*databaseCreateSnapshot)(FDBDatabase* database, uint8_t const* snapshotCommmand, - int snapshotCommandLength); + FDBFuture *(*databaseCreateSnapshot)(FDBDatabase *database, + uint8_t const *uid, int uidLength, + uint8_t const *snapshotCommmand, + int snapshotCommandLength); - //Transaction + //Transaction fdb_error_t (*transactionSetOption)(FDBTransaction *tr, FDBTransactionOptions::Option option, uint8_t const *value, int valueLength); void (*transactionDestroy)(FDBTransaction *tr); @@ -201,9 +203,11 @@ public: ThreadFuture rebootWorker(const StringRef& address, bool check, int duration) override; ThreadFuture forceRecoveryWithDataLoss(const StringRef& dcid) override; - ThreadFuture createSnapshot(const StringRef& snapshot_command) override; + ThreadFuture + createSnapshot(const StringRef &uid, + const StringRef &snapshot_command) override; -private: + private: const Reference api; FdbCApi::FDBDatabase* db; // Always set if API version >= 610, otherwise guaranteed to be set when onReady future is set ThreadFuture ready; @@ -341,9 +345,11 @@ public: ThreadFuture rebootWorker(const StringRef& address, bool check, int duration) override; ThreadFuture forceRecoveryWithDataLoss(const StringRef& dcid) override; - ThreadFuture createSnapshot(const StringRef& snapshot_command) override; + ThreadFuture + createSnapshot(const StringRef &uid, + const StringRef &snapshot_command) override; -private: + private: struct Connector : ThreadCallback, ThreadSafeReferenceCounted { Connector(Reference dbState, Reference client, std::string clusterFilePath) : dbState(dbState), client(client), clusterFilePath(clusterFilePath), connected(false), cancelled(false) {} diff --git a/fdbclient/NativeAPI.actor.cpp b/fdbclient/NativeAPI.actor.cpp index 0e20cb3ae6..429b05905b 100644 --- a/fdbclient/NativeAPI.actor.cpp +++ b/fdbclient/NativeAPI.actor.cpp @@ -4836,16 +4836,19 @@ Future DatabaseContext::forceRecoveryWithDataLoss(StringRef dcId) { return forceRecovery(getConnectionFile(), dcId); } -ACTOR static Future createSnapshotActor(DatabaseContext* cx, StringRef snapCmd) { - UID snapUID = deterministicRandom()->randomUniqueID(); - try { - wait(mgmtSnapCreate(cx->clone(), snapCmd, snapUID)); - } catch (Error& e) { - throw e; - } - return Void(); +ACTOR static Future createSnapshotActor(DatabaseContext *cx, UID snapUID, + StringRef snapCmd) { + wait(mgmtSnapCreate(cx->clone(), snapCmd, snapUID)); + return Void(); } -Future DatabaseContext::createSnapshot(StringRef snapshot_command) { - return createSnapshotActor(this, snapshot_command); +Future DatabaseContext::createSnapshot(StringRef uid, + StringRef snapshot_command) { + std::string uid_str = uid.toString(); + if (!std::all_of(uid_str.begin(), uid_str.end(), + [](unsigned char c) { return std::isxdigit(c); }) || + uid_str.size() != 32) { + throw snap_invalid_uid_string(); + } + return createSnapshotActor(this, UID::fromString(uid_str), snapshot_command); } diff --git a/fdbclient/ThreadSafeTransaction.cpp b/fdbclient/ThreadSafeTransaction.cpp index d589b54ba9..c7fc242106 100644 --- a/fdbclient/ThreadSafeTransaction.cpp +++ b/fdbclient/ThreadSafeTransaction.cpp @@ -84,10 +84,15 @@ ThreadFuture ThreadSafeDatabase::forceRecoveryWithDataLoss(const StringRef } ); } -ThreadFuture ThreadSafeDatabase::createSnapshot(const StringRef& snapshot_command) { - DatabaseContext* db = this->db; - Key cmd = snapshot_command; - return onMainThread([db, cmd]() -> Future { return db->createSnapshot(cmd); }); +ThreadFuture +ThreadSafeDatabase::createSnapshot(const StringRef &uid, + const StringRef &snapshot_command) { + DatabaseContext *db = this->db; + Key snapUID = uid; + Key cmd = snapshot_command; + return onMainThread([db, snapUID, cmd]() -> Future { + return db->createSnapshot(snapUID, cmd); + }); } ThreadSafeDatabase::ThreadSafeDatabase(std::string connFilename, int apiVersion) { diff --git a/fdbclient/ThreadSafeTransaction.h b/fdbclient/ThreadSafeTransaction.h index f8fb35bf9e..21a3fb7751 100644 --- a/fdbclient/ThreadSafeTransaction.h +++ b/fdbclient/ThreadSafeTransaction.h @@ -43,9 +43,11 @@ public: ThreadFuture rebootWorker(const StringRef& address, bool check, int duration) override; ThreadFuture forceRecoveryWithDataLoss(const StringRef& dcid) override; - ThreadFuture createSnapshot(const StringRef& snapshot_command) override; + ThreadFuture + createSnapshot(const StringRef &uid, + const StringRef &snapshot_command) override; -private: + private: friend class ThreadSafeTransaction; DatabaseContext* db; public: // Internal use only diff --git a/flow/error_definitions.h b/flow/error_definitions.h index 20d9855e44..70f8750836 100755 --- a/flow/error_definitions.h +++ b/flow/error_definitions.h @@ -236,6 +236,7 @@ ERROR( snap_path_not_whitelisted, 2505, "Snapshot create binary path not whiteli ERROR( snap_not_fully_recovered_unsupported, 2506, "Unsupported when the cluster is not fully recovered") ERROR( snap_log_anti_quorum_unsupported, 2507, "Unsupported when log anti quorum is configured") ERROR( snap_with_recovery_unsupported, 2508, "Cluster recovery during snapshot operation not supported") +ERROR( snap_invalid_uid_string, 2509, "The given uid string is not a 32-length hex string") // 4xxx Internal errors (those that should be generated only by bugs) are decimal 4xxx ERROR( unknown_error, 4000, "An unknown error occurred" ) // C++ exception not of type Error