From f4c5af525e10479d619555fad3abf29c3436f863 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Wed, 1 Nov 2023 00:11:38 +0800 Subject: [PATCH 1/5] Fix lifecycle setup --- dbms/src/Storages/S3/S3Common.cpp | 4 +++- dbms/src/Storages/S3/S3Common.h | 3 ++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/dbms/src/Storages/S3/S3Common.cpp b/dbms/src/Storages/S3/S3Common.cpp index 2ad7e6528d6..e929dd39fa5 100644 --- a/dbms/src/Storages/S3/S3Common.cpp +++ b/dbms/src/Storages/S3/S3Common.cpp @@ -755,10 +755,12 @@ bool ensureLifecycleRuleExist(const TiFlashS3Client & client, Int32 expire_days) { const auto & error = outcome.GetError(); // The life cycle is not added at all - if (error.GetExceptionName() == "NoSuchLifecycleConfiguration") + if (error.GetErrorType() == Aws::S3::S3Errors::RESOURCE_NOT_FOUND + || error.GetExceptionName() == "NoSuchLifecycleConfiguration") { break; } + LOG_WARNING( client.log, "GetBucketLifecycle fail, please check the bucket lifecycle configuration or create the lifecycle rule " diff --git a/dbms/src/Storages/S3/S3Common.h b/dbms/src/Storages/S3/S3Common.h index f7295901b69..191d5d0a29d 100644 --- a/dbms/src/Storages/S3/S3Common.h +++ b/dbms/src/Storages/S3/S3Common.h @@ -44,8 +44,9 @@ namespace DB::S3 inline String S3ErrorMessage(const Aws::S3::S3Error & e) { return fmt::format( - " s3error={} s3msg={} request_id={}", + " s3error={} s3exception_name={} s3msg={} request_id={}", magic_enum::enum_name(e.GetErrorType()), + e.GetExceptionName(), e.GetMessage(), e.GetRequestId()); } From 668c1a35c2184deb51738ff2d733d2a290fbf81a Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Wed, 1 Nov 2023 00:12:36 +0800 Subject: [PATCH 2/5] Add http hooks for remote info/resign/gc/upload --- dbms/src/Interpreters/Context.cpp | 4 +- dbms/src/Interpreters/Context.h | 2 +- .../KVStore/FFI/ProxyFFIStatusService.cpp | 118 +++++++++++++++--- dbms/src/Storages/KVStore/TMTContext.cpp | 4 +- dbms/src/Storages/KVStore/TMTContext.h | 2 + .../Universal/UniversalPageStorageService.cpp | 12 +- .../Universal/UniversalPageStorageService.h | 4 +- dbms/src/Storages/S3/S3GCManager.cpp | 8 ++ dbms/src/Storages/S3/S3GCManager.h | 2 + 9 files changed, 128 insertions(+), 28 deletions(-) diff --git a/dbms/src/Interpreters/Context.cpp b/dbms/src/Interpreters/Context.cpp index af54d3c3742..d57f48c2396 100644 --- a/dbms/src/Interpreters/Context.cpp +++ b/dbms/src/Interpreters/Context.cpp @@ -1815,12 +1815,12 @@ UniversalPageStoragePtr Context::tryGetWriteNodePageStorage() const return nullptr; } -bool Context::trySyncAllDataToRemoteStore() const +bool Context::tryUploadAllDataToRemoteStore() const { auto lock = getLock(); if (shared->ctx_disagg->isDisaggregatedStorageMode() && shared->ps_write) { - shared->ps_write->setSyncAllData(); + shared->ps_write->setUploadAllData(); return true; } return false; diff --git a/dbms/src/Interpreters/Context.h b/dbms/src/Interpreters/Context.h index 23f1305555b..759bcb25b66 100644 --- a/dbms/src/Interpreters/Context.h +++ b/dbms/src/Interpreters/Context.h @@ -452,7 +452,7 @@ class Context void initializeWriteNodePageStorageIfNeed(const PathPool & path_pool); UniversalPageStoragePtr getWriteNodePageStorage() const; UniversalPageStoragePtr tryGetWriteNodePageStorage() const; - bool trySyncAllDataToRemoteStore() const; + bool tryUploadAllDataToRemoteStore() const; void tryReleaseWriteNodePageStorageForTest(); SharedContextDisaggPtr getSharedContextDisagg() const; diff --git a/dbms/src/Storages/KVStore/FFI/ProxyFFIStatusService.cpp b/dbms/src/Storages/KVStore/FFI/ProxyFFIStatusService.cpp index a2b076d043a..ff561b89310 100644 --- a/dbms/src/Storages/KVStore/FFI/ProxyFFIStatusService.cpp +++ b/dbms/src/Storages/KVStore/FFI/ProxyFFIStatusService.cpp @@ -21,6 +21,8 @@ #include #include #include +#include +#include #include #include @@ -55,7 +57,8 @@ HttpRequestRes HandleHttpRequestSyncStatus( status = HttpRequestStatus::ErrorParam; return HttpRequestRes{ .status = status, - .res = CppStrWithView{.inner = GenRawCppPtr(), .view = BaseBuffView{nullptr, 0}}}; + .res = CppStrWithView{.inner = GenRawCppPtr(), .view = BaseBuffView{nullptr, 0}}, + }; } @@ -79,7 +82,8 @@ HttpRequestRes HandleHttpRequestSyncStatus( if (status != HttpRequestStatus::Ok) return HttpRequestRes{ .status = status, - .res = CppStrWithView{.inner = GenRawCppPtr(), .view = BaseBuffView{nullptr, 0}}}; + .res = CppStrWithView{.inner = GenRawCppPtr(), .view = BaseBuffView{nullptr, 0}}, + }; } auto & tmt = *server->tmt; @@ -143,8 +147,9 @@ HttpRequestRes HandleHttpRequestSyncStatus( return HttpRequestRes{ .status = status, .res = CppStrWithView{ - .inner = GenRawCppPtr(s, RawCppPtrTypeImpl::String), - .view = BaseBuffView{s->data(), s->size()}}}; + .inner = GenRawCppPtr(s, RawCppPtrTypeImpl::String), .view = BaseBuffView{s->data(), s->size()}, + }, + }; } // Return store status of this tiflash node @@ -158,13 +163,14 @@ HttpRequestRes HandleHttpRequestStoreStatus( auto * name = RawCppString::New(IntoStoreStatusName(server->tmt->getStoreStatus(std::memory_order_relaxed))); return HttpRequestRes{ .status = HttpRequestStatus::Ok, - .res = CppStrWithView{ - .inner = GenRawCppPtr(name, RawCppPtrTypeImpl::String), - .view = BaseBuffView{name->data(), name->size()}}}; + .res= CppStrWithView{ + .inner = GenRawCppPtr(name, RawCppPtrTypeImpl::String), .view = BaseBuffView{name->data(), name->size()}, + }, + }; } /// set a flag for upload all PageData to remote store from local UniPS -HttpRequestRes HandleHttpRequestRemoteStoreSync( +HttpRequestRes HandleHttpRequestRemoteReUpload( EngineStoreServerWrap * server, std::string_view, const std::string &, @@ -179,17 +185,95 @@ HttpRequestRes HandleHttpRequestRemoteStoreSync( magic_enum::enum_name(global_ctx.getSharedContextDisagg()->disaggregated_mode))); return HttpRequestRes{ .status = HttpRequestStatus::ErrorParam, - .res - = CppStrWithView{.inner = GenRawCppPtr(body, RawCppPtrTypeImpl::String), .view = BaseBuffView{body->data(), body->size()}}, + .res = CppStrWithView{ + .inner = GenRawCppPtr(body, RawCppPtrTypeImpl::String), + .view = BaseBuffView{body->data(), body->size()}, + }, }; } - bool flag_set = global_ctx.trySyncAllDataToRemoteStore(); + bool flag_set = global_ctx.tryUploadAllDataToRemoteStore(); auto * body = RawCppString::New(fmt::format(R"json({{"message":"flag_set={}"}})json", flag_set)); return HttpRequestRes{ .status = flag_set ? HttpRequestStatus::Ok : HttpRequestStatus::ErrorParam, - .res - = CppStrWithView{.inner = GenRawCppPtr(body, RawCppPtrTypeImpl::String), .view = BaseBuffView{body->data(), body->size()}}, + .res = CppStrWithView{ + .inner = GenRawCppPtr(body, RawCppPtrTypeImpl::String), + .view = BaseBuffView{body->data(), body->size()}, + }, + }; +} + +// get the remote gc owner info +HttpRequestRes HandleHttpRequestRemoteOwnerInfo( + EngineStoreServerWrap * server, + std::string_view, + const std::string &, + std::string_view, + std::string_view) +{ + const auto & owner = server->tmt->getS3GCOwnerManager(); + const auto owner_info = owner->getOwnerID(); + auto * body = RawCppString::New(fmt::format( + R"json({{"status":"{}","owner_id":"{}"}})json", + magic_enum::enum_name(owner_info.status), + owner_info.owner_id)); + return HttpRequestRes{ + .status = HttpRequestStatus::Ok, + .res = CppStrWithView{ + .inner = GenRawCppPtr(body, RawCppPtrTypeImpl::String), + .view = BaseBuffView{body->data(), body->size()}, + }, + }; +} + +// resign if this node is the remote gc owner +HttpRequestRes HandleHttpRequestRemoteOwnerResign( + EngineStoreServerWrap * server, + std::string_view, + const std::string &, + std::string_view, + std::string_view) +{ + const auto & owner = server->tmt->getS3GCOwnerManager(); + bool has_resign = owner->resignOwner(); + String msg = has_resign ? "Done" : "This node is not the remote gc owner, can't be resigned."; + auto * body = RawCppString::New(fmt::format(R"json({{"message":"{}"}})json", msg)); + return HttpRequestRes{ + .status = HttpRequestStatus::Ok, + .res = CppStrWithView{ + .inner = GenRawCppPtr(body, RawCppPtrTypeImpl::String), + .view = BaseBuffView{body->data(), body->size()}, + }, + }; +} + +// execute remote gc if this node is the remote gc owner +HttpRequestRes HandleHttpRequestRemoteGC( + EngineStoreServerWrap * server, + std::string_view, + const std::string &, + std::string_view, + std::string_view) +{ + const auto & owner = server->tmt->getS3GCOwnerManager(); + const auto owner_info = owner->getOwnerID(); + bool gc_is_executed = false; + if (owner_info.status == OwnerType::IsOwner) + { + server->tmt->getS3GCManager()->wake(); + gc_is_executed = true; + } + auto * body = RawCppString::New(fmt::format( + R"json({{"status":"{}","owner_id":"{}","execute":"{}"}})json", + magic_enum::enum_name(owner_info.status), + owner_info.owner_id, + gc_is_executed)); + return HttpRequestRes{ + .status = HttpRequestStatus::Ok, + .res = CppStrWithView{ + .inner = GenRawCppPtr(body, RawCppPtrTypeImpl::String), + .view = BaseBuffView{body->data(), body->size()}, + }, }; } @@ -203,7 +287,10 @@ using HANDLE_HTTP_URI_METHOD = HttpRequestRes (*)( static const std::map AVAILABLE_HTTP_URI = { {"/tiflash/sync-status/", HandleHttpRequestSyncStatus}, {"/tiflash/store-status", HandleHttpRequestStoreStatus}, - {"/tiflash/sync-remote-store", HandleHttpRequestRemoteStoreSync}, + {"/tiflash/remote/owner/info", HandleHttpRequestRemoteOwnerInfo}, + {"/tiflash/remote/owner/resign", HandleHttpRequestRemoteOwnerResign}, + {"/tiflash/remote/gc", HandleHttpRequestRemoteGC}, + {"/tiflash/remote/upload", HandleHttpRequestRemoteReUpload}, }; uint8_t CheckHttpUriAvailable(BaseBuffView path_) @@ -239,7 +326,8 @@ HttpRequestRes HandleHttpRequest( } return HttpRequestRes{ .status = HttpRequestStatus::ErrorParam, - .res = CppStrWithView{.inner = GenRawCppPtr(), .view = BaseBuffView{nullptr, 0}}}; + .res = CppStrWithView{.inner = GenRawCppPtr(), .view = BaseBuffView{nullptr, 0}}, + }; } } // namespace DB diff --git a/dbms/src/Storages/KVStore/TMTContext.cpp b/dbms/src/Storages/KVStore/TMTContext.cpp index 2d785b33a45..409397b6e32 100644 --- a/dbms/src/Storages/KVStore/TMTContext.cpp +++ b/dbms/src/Storages/KVStore/TMTContext.cpp @@ -190,8 +190,8 @@ TMTContext::TMTContext( magic_enum::enum_name(remote_gc_config.method)); } } - remote_gc_config.interval_seconds - = context.getSettingsRef().remote_gc_interval_seconds; // TODO: make it reloadable + // TODO: make it reloadable + remote_gc_config.interval_seconds = context.getSettingsRef().remote_gc_interval_seconds; remote_gc_config.verify_locks = context.getSettingsRef().remote_gc_verify_consistency > 0; // set the gc_method so that S3LockService can set tagging when create delmark S3::ClientFactory::instance().gc_method = remote_gc_config.method; diff --git a/dbms/src/Storages/KVStore/TMTContext.h b/dbms/src/Storages/KVStore/TMTContext.h index 0be5cbcc331..0cab37f847c 100644 --- a/dbms/src/Storages/KVStore/TMTContext.h +++ b/dbms/src/Storages/KVStore/TMTContext.h @@ -112,6 +112,8 @@ class TMTContext : private boost::noncopyable const OwnerManagerPtr & getS3GCOwnerManager() const; + const S3::S3GCManagerServicePtr & getS3GCManager() const { return s3gc_manager; } + S3::S3LockClientPtr getS3LockClient() const { return s3lock_client; } MPPTaskManagerPtr getMPPTaskManager(); diff --git a/dbms/src/Storages/Page/V3/Universal/UniversalPageStorageService.cpp b/dbms/src/Storages/Page/V3/Universal/UniversalPageStorageService.cpp index 29a22816402..992b54202fb 100644 --- a/dbms/src/Storages/Page/V3/Universal/UniversalPageStorageService.cpp +++ b/dbms/src/Storages/Page/V3/Universal/UniversalPageStorageService.cpp @@ -98,9 +98,9 @@ bool CheckpointUploadFunctor::operator()(const PS::V3::LocalCheckpointFiles & ch return remote_store->putCheckpointFiles(checkpoint, store_id, sequence); } -void UniversalPageStorageService::setSyncAllData() +void UniversalPageStorageService::setUploadAllData() { - sync_all_at_next_upload = true; + upload_all_at_next_upload = true; gc_handle->wake(); LOG_INFO(log, "sync_all flag is set, next checkpoint will upload all existing data"); } @@ -144,10 +144,10 @@ bool UniversalPageStorageService::uploadCheckpoint() return false; } auto s3lock_client = tmt.getS3LockClient(); - const bool force_sync = sync_all_at_next_upload.load(); - bool upload_done = uploadCheckpointImpl(store_info, s3lock_client, remote_store, force_sync); - if (force_sync && upload_done) - sync_all_at_next_upload = false; + const bool force_upload = upload_all_at_next_upload.load(); + bool upload_done = uploadCheckpointImpl(store_info, s3lock_client, remote_store, force_upload); + if (force_upload && upload_done) + upload_all_at_next_upload = false; // always return false to run at fixed rate return false; } diff --git a/dbms/src/Storages/Page/V3/Universal/UniversalPageStorageService.h b/dbms/src/Storages/Page/V3/Universal/UniversalPageStorageService.h index 8563ea4a6c0..04b4bf0a225 100644 --- a/dbms/src/Storages/Page/V3/Universal/UniversalPageStorageService.h +++ b/dbms/src/Storages/Page/V3/Universal/UniversalPageStorageService.h @@ -52,7 +52,7 @@ class UniversalPageStorageService final bool uploadCheckpoint(); // Set a flag for sync all data to remote store at next checkpoint - void setSyncAllData(); + void setUploadAllData(); UniversalPageStoragePtr getUniversalPageStorage() const { return uni_page_storage; } ~UniversalPageStorageService(); @@ -90,7 +90,7 @@ class UniversalPageStorageService final // Once this flag is set, all data will be synced to remote store at next time // `uploadCheckpoint` is called. - std::atomic_bool sync_all_at_next_upload{false}; + std::atomic_bool upload_all_at_next_upload{false}; Context & global_context; UniversalPageStoragePtr uni_page_storage; diff --git a/dbms/src/Storages/S3/S3GCManager.cpp b/dbms/src/Storages/S3/S3GCManager.cpp index 7c571953cc6..0c660e2ca28 100644 --- a/dbms/src/Storages/S3/S3GCManager.cpp +++ b/dbms/src/Storages/S3/S3GCManager.cpp @@ -765,4 +765,12 @@ void S3GCManagerService::shutdown() } } +void S3GCManagerService::wake() const +{ + if (timer) + { + timer->wake(); + } +} + } // namespace DB::S3 diff --git a/dbms/src/Storages/S3/S3GCManager.h b/dbms/src/Storages/S3/S3GCManager.h index 1d097bb4644..b411c93f049 100644 --- a/dbms/src/Storages/S3/S3GCManager.h +++ b/dbms/src/Storages/S3/S3GCManager.h @@ -165,6 +165,8 @@ class S3GCManagerService void shutdown(); + void wake() const; + private: Context & global_ctx; std::unique_ptr manager; From 388e44dcdef49b35f7e028587f07d5ec4b3f4f15 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Wed, 1 Nov 2023 13:13:21 +0800 Subject: [PATCH 3/5] temp save --- dbms/src/Storages/S3/S3Common.cpp | 1 + dbms/src/Storages/S3/tests/gtest_s3client.cpp | 39 +++++++++++++++---- 2 files changed, 33 insertions(+), 7 deletions(-) diff --git a/dbms/src/Storages/S3/S3Common.cpp b/dbms/src/Storages/S3/S3Common.cpp index e929dd39fa5..0b8718ee8f0 100644 --- a/dbms/src/Storages/S3/S3Common.cpp +++ b/dbms/src/Storages/S3/S3Common.cpp @@ -1132,6 +1132,7 @@ void rawListPrefix( { req.SetDelimiter(String(delimiter)); } + req.SetEncodingType(Aws::S3::Model::EncodingType::url); static auto log = Logger::get("S3RawListPrefix"); diff --git a/dbms/src/Storages/S3/tests/gtest_s3client.cpp b/dbms/src/Storages/S3/tests/gtest_s3client.cpp index dbde5a68935..15cf27455b9 100644 --- a/dbms/src/Storages/S3/tests/gtest_s3client.cpp +++ b/dbms/src/Storages/S3/tests/gtest_s3client.cpp @@ -27,8 +27,10 @@ class S3ClientTest : public ::testing::Test void SetUp() override { client = ClientFactory::instance().sharedTiFlashClient(); +#if 0 ::DB::tests::TiFlashTestEnv::deleteBucket(*client); ::DB::tests::TiFlashTestEnv::createBucketIfNotExist(*client); +#endif } std::shared_ptr client; @@ -44,6 +46,7 @@ CATCH TEST_F(S3ClientTest, UploadRead) try { +#if 0 deleteObject(*client, "s999/manifest/mf_1"); ASSERT_FALSE(objectExists(*client, "s999/manifest/mf_1")); uploadEmptyFile(*client, "s999/manifest/mf_1"); @@ -56,18 +59,28 @@ try uploadEmptyFile(*client, "s999/data/dat_790_0"); uploadEmptyFile(*client, "s999/abcd"); +#endif { Strings prefixes; - listPrefixWithDelimiter(*client, "s999/", "/", [&](const Aws::S3::Model::CommonPrefix & p) { - prefixes.emplace_back(p.GetPrefix()); - return PageResult{.num_keys = 1, .more = true}; - }); + rawListPrefix( + *client, + client->bucket(), + "tiflash_ut/", + "/", + [&](const Aws::S3::Model::ListObjectsV2Result & result) { + const auto & ps = result.GetCommonPrefixes(); + for (const auto & p : ps) + { + prefixes.emplace_back(p.GetPrefix()); + } + return PageResult{.num_keys = ps.size(), .more = true}; + }); ASSERT_EQ(prefixes.size(), 2) << fmt::format("{}", prefixes); - EXPECT_EQ(prefixes[0], "s999/data/"); - EXPECT_EQ(prefixes[1], "s999/manifest/"); + EXPECT_EQ(prefixes[0], client->root() + "s999/data/"); + EXPECT_EQ(prefixes[1], client->root() + "s999/manifest/"); } - +#if 0 // check the keys with raw `LIST` request { Strings prefixes; @@ -88,6 +101,18 @@ try EXPECT_EQ(prefixes[0], client->root() + "s999/data/"); EXPECT_EQ(prefixes[1], client->root() + "s999/manifest/"); } + + { + Strings prefixes; + listPrefixWithDelimiter(*client, "s999/", "/", [&](const Aws::S3::Model::CommonPrefix & p) { + prefixes.emplace_back(p.GetPrefix()); + return PageResult{.num_keys = 1, .more = true}; + }); + ASSERT_EQ(prefixes.size(), 2) << fmt::format("{}", prefixes); + EXPECT_EQ(prefixes[0], "s999/data/"); + EXPECT_EQ(prefixes[1], "s999/manifest/"); + } +#endif } CATCH From 76aeaa3622e121ac8d27050cb007933c69ac9b88 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Wed, 1 Nov 2023 14:16:33 +0800 Subject: [PATCH 4/5] Fix S3 LIST always return empty result --- .gitmodules | 3 + ...y-to-check-if-there-is-anything-in-r.patch | 101 ++++++++++++++++++ ...rror-logging-and-404-for-HEAD-reques.patch | 62 +++++++++++ contrib/aws-cmake/CMakeLists.txt | 39 ++++++- 4 files changed, 204 insertions(+), 1 deletion(-) create mode 100644 contrib/aws-cmake/0001-More-reliable-way-to-check-if-there-is-anything-in-r.patch create mode 100644 contrib/aws-cmake/0002-Reduce-verbose-error-logging-and-404-for-HEAD-reques.patch diff --git a/.gitmodules b/.gitmodules index a1c0762a723..021da59cf22 100644 --- a/.gitmodules +++ b/.gitmodules @@ -95,6 +95,9 @@ [submodule "contrib/aws"] path = contrib/aws url = https://github.com/aws/aws-sdk-cpp.git + # ignore dirty status since we apply patches to + # fix bug when using Poco client + ignore = dirty [submodule "contrib/aws-c-auth"] path = contrib/aws-c-auth url = https://github.com/awslabs/aws-c-auth.git diff --git a/contrib/aws-cmake/0001-More-reliable-way-to-check-if-there-is-anything-in-r.patch b/contrib/aws-cmake/0001-More-reliable-way-to-check-if-there-is-anything-in-r.patch new file mode 100644 index 00000000000..495f81fe90d --- /dev/null +++ b/contrib/aws-cmake/0001-More-reliable-way-to-check-if-there-is-anything-in-r.patch @@ -0,0 +1,101 @@ +From 92225a50ccbab5369fd40b99a310ef7fcaec1750 Mon Sep 17 00:00:00 2001 +From: JaySon-Huang +Date: Thu, 6 Apr 2023 12:54:23 +0800 +Subject: [PATCH 1/2] More reliable way to check if there is anything in result + IOStream + +Signed-off-by: JaySon-Huang +--- + src/aws-cpp-sdk-core/source/client/AWSJsonClient.cpp | 9 +++++---- + src/aws-cpp-sdk-core/source/client/AWSXmlClient.cpp | 6 +++--- + .../source/internal/AWSHttpResourceClient.cpp | 2 +- + 3 files changed, 9 insertions(+), 8 deletions(-) + +diff --git a/src/aws-cpp-sdk-core/source/client/AWSJsonClient.cpp b/src/aws-cpp-sdk-core/source/client/AWSJsonClient.cpp +index f42a306156..3cd26203f0 100644 +--- a/src/aws-cpp-sdk-core/source/client/AWSJsonClient.cpp ++++ b/src/aws-cpp-sdk-core/source/client/AWSJsonClient.cpp +@@ -115,7 +115,8 @@ JsonOutcome AWSJsonClient::MakeRequest(const Aws::Http::URI& uri, + {{TracingUtils::SMITHY_METHOD_DIMENSION, request.GetServiceRequestName()}, {TracingUtils::SMITHY_SERVICE_DIMENSION, this->GetServiceClientName()}}); + } + +- if (httpOutcome.GetResult()->GetResponseBody().tellp() > 0){ ++ if (httpOutcome.GetResult()->GetResponseBody().peek() != std::char_traits::eof()) ++ { + return smithy::components::tracing::TracingUtils::MakeCallWithTiming( + [&]() -> JsonOutcome { + return JsonOutcome(AmazonWebServiceResult(JsonValue(httpOutcome.GetResult()->GetResponseBody()), +@@ -154,7 +155,7 @@ JsonOutcome AWSJsonClient::MakeRequest(const Aws::Http::URI& uri, + {{TracingUtils::SMITHY_METHOD_DIMENSION, requestName}, {TracingUtils::SMITHY_SERVICE_DIMENSION, this->GetServiceClientName()}}); + } + +- if (httpOutcome.GetResult()->GetResponseBody().tellp() > 0) ++ if (httpOutcome.GetResult()->GetResponseBody().peek() != std::char_traits::eof()) + { + JsonValue jsonValue(httpOutcome.GetResult()->GetResponseBody()); + if (!jsonValue.WasParseSuccessful()) { +@@ -203,7 +204,7 @@ JsonOutcome AWSJsonClient::MakeEventStreamRequest(std::shared_ptrGetResponseBody().tellp() > 0) ++ if (httpOutcome.GetResult()->GetResponseBody().peek() != std::char_traits::eof()) + { + JsonValue jsonValue(httpOutcome.GetResult()->GetResponseBody()); + if (!jsonValue.WasParseSuccessful()) +@@ -229,7 +230,7 @@ AWSError AWSJsonClient::BuildAWSError( + bool retryable = httpResponse->GetClientErrorType() == CoreErrors::NETWORK_CONNECTION ? true : false; + error = AWSError(httpResponse->GetClientErrorType(), "", httpResponse->GetClientErrorMessage(), retryable); + } +- else if (!httpResponse->GetResponseBody() || httpResponse->GetResponseBody().tellp() < 1) ++ else if (!httpResponse->GetResponseBody() || httpResponse->GetResponseBody().peek() == std::char_traits::eof()) + { + auto responseCode = httpResponse->GetResponseCode(); + auto errorCode = AWSClient::GuessBodylessErrorType(responseCode); +diff --git a/src/aws-cpp-sdk-core/source/client/AWSXmlClient.cpp b/src/aws-cpp-sdk-core/source/client/AWSXmlClient.cpp +index 443ea31cbc..c122c5d5a1 100644 +--- a/src/aws-cpp-sdk-core/source/client/AWSXmlClient.cpp ++++ b/src/aws-cpp-sdk-core/source/client/AWSXmlClient.cpp +@@ -110,7 +110,7 @@ XmlOutcome AWSXMLClient::MakeRequest(const Aws::Http::URI& uri, + {{TracingUtils::SMITHY_METHOD_DIMENSION, request.GetServiceRequestName()}, {TracingUtils::SMITHY_SERVICE_DIMENSION, this->GetServiceClientName()}}); + } + +- if (httpOutcome.GetResult()->GetResponseBody().tellp() > 0) ++ if (httpOutcome.GetResult()->GetResponseBody().peek() != std::char_traits::eof()) + { + return smithy::components::tracing::TracingUtils::MakeCallWithTiming( + [&]() -> XmlOutcome { +@@ -152,7 +152,7 @@ XmlOutcome AWSXMLClient::MakeRequest(const Aws::Http::URI& uri, + {{TracingUtils::SMITHY_METHOD_DIMENSION, requestName}, {TracingUtils::SMITHY_SERVICE_DIMENSION, this->GetServiceClientName()}}); + } + +- if (httpOutcome.GetResult()->GetResponseBody().tellp() > 0) ++ if (httpOutcome.GetResult()->GetResponseBody().peek() != std::char_traits::eof()) + { + return smithy::components::tracing::TracingUtils::MakeCallWithTiming( + [&]() -> XmlOutcome { +@@ -182,7 +182,7 @@ AWSError AWSXMLClient::BuildAWSError(const std::shared_ptrGetClientErrorType() == CoreErrors::NETWORK_CONNECTION ? true : false; + error = AWSError(httpResponse->GetClientErrorType(), "", httpResponse->GetClientErrorMessage(), retryable); + } +- else if (!httpResponse->GetResponseBody() || httpResponse->GetResponseBody().tellp() < 1) ++ else if (!httpResponse->GetResponseBody() || httpResponse->GetResponseBody().peek() == std::char_traits::eof()) + { + auto responseCode = httpResponse->GetResponseCode(); + auto errorCode = AWSClient::GuessBodylessErrorType(responseCode); +diff --git a/src/aws-cpp-sdk-core/source/internal/AWSHttpResourceClient.cpp b/src/aws-cpp-sdk-core/source/internal/AWSHttpResourceClient.cpp +index 723747bbf1..8d84083ba3 100644 +--- a/src/aws-cpp-sdk-core/source/internal/AWSHttpResourceClient.cpp ++++ b/src/aws-cpp-sdk-core/source/internal/AWSHttpResourceClient.cpp +@@ -148,7 +148,7 @@ namespace Aws + AWS_LOGSTREAM_ERROR(m_logtag.c_str(), "Http request to retrieve credentials failed"); + return AWSError(CoreErrors::NETWORK_CONNECTION, true); // Retryable + } +- else if (m_errorMarshaller && response->GetResponseBody().tellp() > 0) ++ else if (m_errorMarshaller && response->GetResponseBody().peek() != std::char_traits::eof()) + { + return m_errorMarshaller->Marshall(*response); + } +-- +2.31.1 + diff --git a/contrib/aws-cmake/0002-Reduce-verbose-error-logging-and-404-for-HEAD-reques.patch b/contrib/aws-cmake/0002-Reduce-verbose-error-logging-and-404-for-HEAD-reques.patch new file mode 100644 index 00000000000..30dfdba9c4c --- /dev/null +++ b/contrib/aws-cmake/0002-Reduce-verbose-error-logging-and-404-for-HEAD-reques.patch @@ -0,0 +1,62 @@ +From 7e6f90112f21c6996e097012c0fe6bfc5c3445d3 Mon Sep 17 00:00:00 2001 +From: JaySon-Huang +Date: Wed, 17 May 2023 15:56:17 +0800 +Subject: [PATCH 2/2] Reduce verbose error logging and 404 for HEAD request + +--- + src/aws-cpp-sdk-core/source/client/AWSClient.cpp | 2 +- + src/aws-cpp-sdk-core/source/client/AWSXmlClient.cpp | 12 +++++++++++- + 2 files changed, 12 insertions(+), 2 deletions(-) + +diff --git a/src/aws-cpp-sdk-core/source/client/AWSClient.cpp b/src/aws-cpp-sdk-core/source/client/AWSClient.cpp +index 5d8a7a9e8a..932bf7d2c0 100644 +--- a/src/aws-cpp-sdk-core/source/client/AWSClient.cpp ++++ b/src/aws-cpp-sdk-core/source/client/AWSClient.cpp +@@ -209,7 +209,6 @@ bool AWSClient::AdjustClockSkew(HttpResponseOutcome& outcome, const char* signer + { + auto signer = GetSignerByName(signerName); + //detect clock skew and try to correct. +- AWS_LOGSTREAM_WARN(AWS_CLIENT_LOG_TAG, "If the signature check failed. This could be because of a time skew. Attempting to adjust the signer."); + + DateTime serverTime = GetServerTimeFromError(outcome.GetError()); + const auto signingTimestamp = signer->GetSigningTimestamp(); +@@ -224,6 +223,7 @@ bool AWSClient::AdjustClockSkew(HttpResponseOutcome& outcome, const char* signer + //only try again if clock skew was the cause of the error. + if (diff >= TIME_DIFF_MAX || diff <= TIME_DIFF_MIN) + { ++ AWS_LOGSTREAM_WARN(AWS_CLIENT_LOG_TAG, "If the signature check failed. This could be because of a time skew. Attempting to adjust the signer."); + diff = DateTime::Diff(serverTime, DateTime::Now()); + AWS_LOGSTREAM_INFO(AWS_CLIENT_LOG_TAG, "Computed time difference as " << diff.count() << " milliseconds. Adjusting signer with the skew."); + signer->SetClockSkew(diff); +diff --git a/src/aws-cpp-sdk-core/source/client/AWSXmlClient.cpp b/src/aws-cpp-sdk-core/source/client/AWSXmlClient.cpp +index c122c5d5a1..311b64f4c0 100644 +--- a/src/aws-cpp-sdk-core/source/client/AWSXmlClient.cpp ++++ b/src/aws-cpp-sdk-core/source/client/AWSXmlClient.cpp +@@ -13,6 +13,7 @@ + #include + #include + #include ++#include + #include + #include + #include +@@ -207,6 +208,15 @@ AWSError AWSXMLClient::BuildAWSError(const std::shared_ptrGetHeaders()); + error.SetResponseCode(httpResponse->GetResponseCode()); + error.SetRemoteHostIpAddress(httpResponse->GetOriginatingRequest().GetResolvedRemoteHost()); +- AWS_LOGSTREAM_ERROR(AWS_XML_CLIENT_LOG_TAG, error); ++ ++ if (httpResponse->GetOriginatingRequest().GetMethod() == HttpMethod::HTTP_HEAD && httpResponse->GetResponseCode() == HttpResponseCode::NOT_FOUND) ++ { ++ // ignore error logging for HEAD request with 404 response code, ususally it is caused by determining whether the object exists or not. ++ AWS_LOGSTREAM_DEBUG(AWS_XML_CLIENT_LOG_TAG, error); ++ } ++ else ++ { ++ AWS_LOGSTREAM_ERROR(AWS_XML_CLIENT_LOG_TAG, error); ++ } + return error; + } +-- +2.31.1 + diff --git a/contrib/aws-cmake/CMakeLists.txt b/contrib/aws-cmake/CMakeLists.txt index 31504b0aad3..5fe5efdc7d6 100644 --- a/contrib/aws-cmake/CMakeLists.txt +++ b/contrib/aws-cmake/CMakeLists.txt @@ -12,7 +12,8 @@ set(AWS_PUBLIC_COMPILE_DEFS) set(AWS_PRIVATE_COMPILE_DEFS) set(AWS_PRIVATE_LIBS) -# Versions +# Versions. Note that we may update the patch under /aws-cmake to make it work +# after upgrading the aws-sdk-cpp version. list(APPEND AWS_PUBLIC_COMPILE_DEFS "-DAWS_SDK_VERSION_MAJOR=1") list(APPEND AWS_PUBLIC_COMPILE_DEFS "-DAWS_SDK_VERSION_MINOR=11") list(APPEND AWS_PUBLIC_COMPILE_DEFS "-DAWS_SDK_VERSION_PATCH=186") @@ -91,6 +92,42 @@ file(GLOB AWS_SDK_CORE_SRC "${AWS_SDK_CORE_DIR}/source/utils/xml/*.cpp" ) +execute_process( + COMMAND grep "GetResponseBody().peek()" "${AWS_SDK_CORE_DIR}/source/client/AWSXmlClient.cpp" + RESULT_VARIABLE HAVE_APPLY_PATCH) +# grep - Normally, the exit status is 0 if selected lines are found and 1 otherwise. But the exit status is 2 if an error occurred. +if (HAVE_APPLY_PATCH EQUAL 1) + message(STATUS "aws patch not apply: ${HAVE_APPLY_PATCH}, patching...") + ## update the patch using `git format-patch` if you upgrade aws + set (AWS_PATCH_FILE "${TiFlash_SOURCE_DIR}/contrib/aws-cmake/0001-More-reliable-way-to-check-if-there-is-anything-in-r.patch") + # apply the patch + execute_process( + COMMAND git apply -v "${AWS_PATCH_FILE}" + WORKING_DIRECTORY "${AWS_SDK_CORE_DIR}" + COMMAND_ECHO STDOUT + RESULT_VARIABLE PATCH_SUCC) + if (NOT PATCH_SUCC EQUAL 0) + message(FATAL_ERROR "Can not apply aws patch ${AWS_PATCH_FILE}") + endif () + + set (AWS_PATCH_FILE "${TiFlash_SOURCE_DIR}/contrib/aws-cmake/0002-Reduce-verbose-error-logging-and-404-for-HEAD-reques.patch") + # apply the patch + execute_process( + COMMAND git apply -v "${AWS_PATCH_FILE}" + WORKING_DIRECTORY "${AWS_SDK_CORE_DIR}" + COMMAND_ECHO STDOUT + RESULT_VARIABLE PATCH_SUCC) + if (NOT PATCH_SUCC EQUAL 0) + message(FATAL_ERROR "Can not apply aws patch ${AWS_PATCH_FILE}") + else () + message(STATUS "aws patch done") + endif () +elseif (HAVE_APPLY_PATCH EQUAL 0) + message(STATUS "aws patch have been applied: ${HAVE_APPLY_PATCH}") +else () + message(FATAL_ERROR "Can not check the aws patch status") +endif () + if(OS_LINUX OR OS_DARWIN) file(GLOB AWS_SDK_CORE_NET_SRC "${AWS_SDK_CORE_DIR}/source/net/linux-shared/*.cpp") file(GLOB AWS_SDK_CORE_PLATFORM_SRC "${AWS_SDK_CORE_DIR}/source/platform/linux-shared/*.cpp") From 8f1ab8dbdca9121b98427c19977b53a6d3dd6f85 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Wed, 1 Nov 2023 14:21:31 +0800 Subject: [PATCH 5/5] Revert "temp save" This reverts commit 388e44dcdef49b35f7e028587f07d5ec4b3f4f15. --- dbms/src/Storages/S3/S3Common.cpp | 1 - dbms/src/Storages/S3/tests/gtest_s3client.cpp | 39 ++++--------------- 2 files changed, 7 insertions(+), 33 deletions(-) diff --git a/dbms/src/Storages/S3/S3Common.cpp b/dbms/src/Storages/S3/S3Common.cpp index 0b8718ee8f0..e929dd39fa5 100644 --- a/dbms/src/Storages/S3/S3Common.cpp +++ b/dbms/src/Storages/S3/S3Common.cpp @@ -1132,7 +1132,6 @@ void rawListPrefix( { req.SetDelimiter(String(delimiter)); } - req.SetEncodingType(Aws::S3::Model::EncodingType::url); static auto log = Logger::get("S3RawListPrefix"); diff --git a/dbms/src/Storages/S3/tests/gtest_s3client.cpp b/dbms/src/Storages/S3/tests/gtest_s3client.cpp index 15cf27455b9..dbde5a68935 100644 --- a/dbms/src/Storages/S3/tests/gtest_s3client.cpp +++ b/dbms/src/Storages/S3/tests/gtest_s3client.cpp @@ -27,10 +27,8 @@ class S3ClientTest : public ::testing::Test void SetUp() override { client = ClientFactory::instance().sharedTiFlashClient(); -#if 0 ::DB::tests::TiFlashTestEnv::deleteBucket(*client); ::DB::tests::TiFlashTestEnv::createBucketIfNotExist(*client); -#endif } std::shared_ptr client; @@ -46,7 +44,6 @@ CATCH TEST_F(S3ClientTest, UploadRead) try { -#if 0 deleteObject(*client, "s999/manifest/mf_1"); ASSERT_FALSE(objectExists(*client, "s999/manifest/mf_1")); uploadEmptyFile(*client, "s999/manifest/mf_1"); @@ -59,28 +56,18 @@ try uploadEmptyFile(*client, "s999/data/dat_790_0"); uploadEmptyFile(*client, "s999/abcd"); -#endif { Strings prefixes; - rawListPrefix( - *client, - client->bucket(), - "tiflash_ut/", - "/", - [&](const Aws::S3::Model::ListObjectsV2Result & result) { - const auto & ps = result.GetCommonPrefixes(); - for (const auto & p : ps) - { - prefixes.emplace_back(p.GetPrefix()); - } - return PageResult{.num_keys = ps.size(), .more = true}; - }); + listPrefixWithDelimiter(*client, "s999/", "/", [&](const Aws::S3::Model::CommonPrefix & p) { + prefixes.emplace_back(p.GetPrefix()); + return PageResult{.num_keys = 1, .more = true}; + }); ASSERT_EQ(prefixes.size(), 2) << fmt::format("{}", prefixes); - EXPECT_EQ(prefixes[0], client->root() + "s999/data/"); - EXPECT_EQ(prefixes[1], client->root() + "s999/manifest/"); + EXPECT_EQ(prefixes[0], "s999/data/"); + EXPECT_EQ(prefixes[1], "s999/manifest/"); } -#if 0 + // check the keys with raw `LIST` request { Strings prefixes; @@ -101,18 +88,6 @@ try EXPECT_EQ(prefixes[0], client->root() + "s999/data/"); EXPECT_EQ(prefixes[1], client->root() + "s999/manifest/"); } - - { - Strings prefixes; - listPrefixWithDelimiter(*client, "s999/", "/", [&](const Aws::S3::Model::CommonPrefix & p) { - prefixes.emplace_back(p.GetPrefix()); - return PageResult{.num_keys = 1, .more = true}; - }); - ASSERT_EQ(prefixes.size(), 2) << fmt::format("{}", prefixes); - EXPECT_EQ(prefixes[0], "s999/data/"); - EXPECT_EQ(prefixes[1], "s999/manifest/"); - } -#endif } CATCH