disagg: Fix lifecycle set on AWS S3 and add more http hooks (#8284)

close #8282, close #8288
pingcap · Nov 1, 2023 · e597b78 · e597b78
1 parent a55a10f
commit e597b78
Show file tree

Hide file tree

Showing 15 changed files with 337 additions and 31 deletions.
diff --git 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
@@ -0,0 +1,101 @@
+From 92225a50ccbab5369fd40b99a310ef7fcaec1750 Mon Sep 17 00:00:00 2001
+From: JaySon-Huang <[email protected]>
+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 <[email protected]>
+---
+ 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<char>::eof())
++    {
+         return smithy::components::tracing::TracingUtils::MakeCallWithTiming<JsonOutcome>(
+             [&]() -> JsonOutcome {
+                 return JsonOutcome(AmazonWebServiceResult<JsonValue>(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<char>::eof())
+     {
+         JsonValue jsonValue(httpOutcome.GetResult()->GetResponseBody());
+         if (!jsonValue.WasParseSuccessful()) {
+@@ -203,7 +204,7 @@ JsonOutcome AWSJsonClient::MakeEventStreamRequest(std::shared_ptr<Aws::Http::Htt
+
+     HttpResponseOutcome httpOutcome(std::move(httpResponse));
+
+-    if (httpOutcome.GetResult()->GetResponseBody().tellp() > 0)
++    if (httpOutcome.GetResult()->GetResponseBody().peek() != std::char_traits<char>::eof())
+     {
+         JsonValue jsonValue(httpOutcome.GetResult()->GetResponseBody());
+         if (!jsonValue.WasParseSuccessful())
+@@ -229,7 +230,7 @@ AWSError<CoreErrors> AWSJsonClient::BuildAWSError(
+         bool retryable = httpResponse->GetClientErrorType() == CoreErrors::NETWORK_CONNECTION ? true : false;
+         error = AWSError<CoreErrors>(httpResponse->GetClientErrorType(), "", httpResponse->GetClientErrorMessage(), retryable);
+     }
+-    else if (!httpResponse->GetResponseBody() || httpResponse->GetResponseBody().tellp() < 1)
++    else if (!httpResponse->GetResponseBody() || httpResponse->GetResponseBody().peek() == std::char_traits<char>::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<char>::eof())
+     {
+         return smithy::components::tracing::TracingUtils::MakeCallWithTiming<XmlOutcome>(
+             [&]() -> 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<char>::eof())
+     {
+         return smithy::components::tracing::TracingUtils::MakeCallWithTiming<XmlOutcome>(
+             [&]() -> XmlOutcome {
+@@ -182,7 +182,7 @@ AWSError<CoreErrors> AWSXMLClient::BuildAWSError(const std::shared_ptr<Http::Htt
+         bool retryable = httpResponse->GetClientErrorType() == CoreErrors::NETWORK_CONNECTION ? true : false;
+         error = AWSError<CoreErrors>(httpResponse->GetClientErrorType(), "", httpResponse->GetClientErrorMessage(), retryable);
+     }
+-    else if (!httpResponse->GetResponseBody() || httpResponse->GetResponseBody().tellp() < 1)
++    else if (!httpResponse->GetResponseBody() || httpResponse->GetResponseBody().peek() == std::char_traits<char>::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>(CoreErrors::NETWORK_CONNECTION, true); // Retryable
+                     }
+-                    else if (m_errorMarshaller && response->GetResponseBody().tellp() > 0)
++                    else if (m_errorMarshaller && response->GetResponseBody().peek() != std::char_traits<char>::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
@@ -0,0 +1,62 @@
+From 7e6f90112f21c6996e097012c0fe6bfc5c3445d3 Mon Sep 17 00:00:00 2001
+From: JaySon-Huang <[email protected]>
+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 <aws/core/client/RetryStrategy.h>
+ #include <aws/core/http/HttpClient.h>
+ #include <aws/core/http/HttpResponse.h>
++#include <aws/core/http/HttpTypes.h>
+ #include <aws/core/http/URI.h>
+ #include <aws/core/utils/Outcome.h>
+ #include <aws/core/utils/xml/XmlSerializer.h>
+@@ -207,6 +208,15 @@ AWSError<CoreErrors> AWSXMLClient::BuildAWSError(const std::shared_ptr<Http::Htt
+     error.SetResponseHeaders(httpResponse->GetHeaders());
+     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
@@ -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")

diff --git 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
@@ -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;