Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Nov 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitmodules
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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

Original file line number Diff line number Diff line change
@@ -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

39 changes: 38 additions & 1 deletion contrib/aws-cmake/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down Expand Up @@ -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")
Expand Down
4 changes: 2 additions & 2 deletions dbms/src/Interpreters/Context.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion dbms/src/Interpreters/Context.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Loading