Add hint based on response code

yma11 · May 11, 2024 · 6360d25 · 6360d25
1 parent 1098bd5
commit 6360d25
Show file tree

Hide file tree

Showing 2 changed files with 28 additions and 22 deletions.
diff --git a/velox/connectors/hive/storage_adapters/s3fs/S3FileSystem.cpp b/velox/connectors/hive/storage_adapters/s3fs/S3FileSystem.cpp
@@ -656,7 +656,7 @@ class S3FileSystem::Impl {
         if (maxAttempts.has_value()) {
           VELOX_USER_CHECK(
               (maxAttempts.value() > 0),
-              "Invalid configuration: specify 'max-attempts' > 0.");
+              "Invalid configuration: specify 'hive.s3.max-attempts' > 0.");
           return std::make_shared<Aws::Client::StandardRetryStrategy>(
               maxAttempts.value());
         } else {
@@ -667,7 +667,7 @@ class S3FileSystem::Impl {
         if (maxAttempts.has_value()) {
           VELOX_USER_CHECK(
               (maxAttempts.value() > 0),
-              "Invalid configuration: specify 'max-attempts' > 0.");
+              "Invalid configuration: specify 'hive.s3.max-attempts' > 0.");
           return std::make_shared<Aws::Client::AdaptiveRetryStrategy>(
               maxAttempts.value());
         } else {
@@ -678,7 +678,7 @@ class S3FileSystem::Impl {
         if (maxAttempts.has_value()) {
           VELOX_USER_CHECK(
               (maxAttempts.value() > 0),
-              "Invalid configuration: specify 'max-attempts' > 0.");
+              "Invalid configuration: specify 'hive.s3.max-attempts' > 0.");
           return std::make_shared<Aws::Client::DefaultRetryStrategy>(
               maxAttempts.value());
         } else {

diff --git a/velox/connectors/hive/storage_adapters/s3fs/S3Util.h b/velox/connectors/hive/storage_adapters/s3fs/S3Util.h
@@ -157,25 +157,31 @@ inline std::string getRequestID(
 } // namespace
 
 /// Only Amazon (amz) and Alibaba (oss) request IDs are supported.
-#define VELOX_CHECK_AWS_OUTCOME(outcome, errorMsgPrefix, bucket, key)                                                          \
-  {                                                                                                                            \
-    if (!outcome.IsSuccess()) {                                                                                                \
-      auto error = outcome.GetError();                                                                                         \
-      auto errMsg = fmt::format(                                                                                               \
-          "{} due to: '{}'. Path:'{}', SDK Error Type:{}, HTTP Status Code:{}, S3 Service:'{}', Message:'{}', RequestID:'{}'", \
-          errorMsgPrefix,                                                                                                      \
-          getErrorStringFromS3Error(error),                                                                                    \
-          s3URI(bucket, key),                                                                                                  \
-          static_cast<int>(error.GetErrorType()),                                                                              \
-          error.GetResponseCode(),                                                                                             \
-          getS3BackendService(error.GetResponseHeaders()),                                                                     \
-          error.GetMessage(),                                                                                                  \
-          getRequestID(error.GetResponseHeaders()));                                                                           \
-      if (error.GetResponseCode() == Aws::Http::HttpResponseCode::NOT_FOUND) {                                                 \
-        VELOX_FILE_NOT_FOUND_ERROR(errMsg);                                                                                    \
-      }                                                                                                                        \
-      VELOX_FAIL(errMsg)                                                                                                       \
-    }                                                                                                                          \
+#define VELOX_CHECK_AWS_OUTCOME(outcome, errorMsgPrefix, bucket, key)                                                           \
+  {                                                                                                                             \
+    if (!outcome.IsSuccess()) {                                                                                                 \
+      auto error = outcome.GetError();                                                                                          \
+      auto errMsg = fmt::format(                                                                                                \
+          "{} due to: '{}'. Path:'{}', SDK Error Type:{}, HTTP Status Code:{}, S3 Service:'{}', Message:'{}', RequestID:'{}'.", \
+          errorMsgPrefix,                                                                                                       \
+          getErrorStringFromS3Error(error),                                                                                     \
+          s3URI(bucket, key),                                                                                                   \
+          static_cast<int>(error.GetErrorType()),                                                                               \
+          error.GetResponseCode(),                                                                                              \
+          getS3BackendService(error.GetResponseHeaders()),                                                                      \
+          error.GetMessage(),                                                                                                   \
+          getRequestID(error.GetResponseHeaders()));                                                                            \
+      if (IsRetryableHttpResponseCode(error.GetResponseCode())) {                                                               \
+        auto retryHint = fmt::format(                                                                                           \
+            " This request gets retriable response and has retried {} times, you may increase 'hive.s3.max-attempts'.",         \
+            outcome.GetRetryCount());                                                                                           \
+        errMsg.append(retryHint);                                                                                               \
+      }                                                                                                                         \
+      if (error.GetResponseCode() == Aws::Http::HttpResponseCode::NOT_FOUND) {                                                  \
+        VELOX_FILE_NOT_FOUND_ERROR(errMsg);                                                                                     \
+      }                                                                                                                         \
+      VELOX_FAIL(errMsg)                                                                                                        \
+    }                                                                                                                           \
   }
 
 bool isHostExcludedFromProxy(