address comments

yma11 · Jun 5, 2024 · 1b3716a · 1b3716a
1 parent 66c1761
commit 1b3716a
Show file tree

Hide file tree

Showing 4 changed files with 23 additions and 14 deletions.
diff --git a/velox/connectors/hive/HiveConfig.cpp b/velox/connectors/hive/HiveConfig.cpp
@@ -120,9 +120,9 @@ std::optional<uint32_t> HiveConfig::s3MaxConnections() const {
       config_->get<uint32_t>(kS3MaxConnections));
 }
 
-std::optional<uint32_t> HiveConfig::s3MaxAttempts() const {
-  return static_cast<std::optional<std::uint32_t>>(
-      config_->get<uint32_t>(kS3MaxAttempts));
+std::optional<int32_t> HiveConfig::s3MaxAttempts() const {
+  return static_cast<std::optional<std::int32_t>>(
+      config_->get<int32_t>(kS3MaxAttempts));
 }
 
 std::optional<std::string> HiveConfig::s3RetryMode() const {

diff --git a/velox/connectors/hive/HiveConfig.h b/velox/connectors/hive/HiveConfig.h
@@ -248,7 +248,7 @@ class HiveConfig {
 
   std::optional<uint32_t> s3MaxConnections() const;
 
-  std::optional<uint32_t> s3MaxAttempts() const;
+  std::optional<int32_t> s3MaxAttempts() const;
 
   std::optional<std::string> s3RetryMode() const;
 

diff --git a/velox/connectors/hive/storage_adapters/s3fs/S3FileSystem.cpp b/velox/connectors/hive/storage_adapters/s3fs/S3FileSystem.cpp
@@ -27,6 +27,7 @@
 #include <glog/logging.h>
 #include <memory>
 #include <stdexcept>
+#include <iostream>
 
 #include <aws/core/Aws.h>
 #include <aws/core/auth/AWSCredentialsProviderChain.h>
@@ -652,11 +653,15 @@ class S3FileSystem::Impl {
     auto retryMode = hiveConfig_->s3RetryMode();
     auto maxAttempts = hiveConfig_->s3MaxAttempts();
     if (retryMode.has_value()) {
+      std::cout << "***Current retry mode is:" << retryMode.value();
+      std::cout << "***Current max attempts is:" << maxAttempts.value();
       if (retryMode.value() == "standard") {
         if (maxAttempts.has_value()) {
-          VELOX_USER_CHECK(
-              (maxAttempts.value() > 0),
-              "Invalid configuration: specify 'hive.s3.max-attempts' > 0.");
+          VELOX_USER_CHECK_GE(
+              maxAttempts.value(),
+              0,
+              "Invalid configuration: specified 'hive.s3.max-attempts' value {} is < 0.",
+              maxAttempts.value());
           return std::make_shared<Aws::Client::StandardRetryStrategy>(
               maxAttempts.value());
         } else {
@@ -665,9 +670,11 @@ class S3FileSystem::Impl {
         }
       } else if (retryMode.value() == "adaptive") {
         if (maxAttempts.has_value()) {
-          VELOX_USER_CHECK(
-              (maxAttempts.value() > 0),
-              "Invalid configuration: specify 'hive.s3.max-attempts' > 0.");
+          VELOX_USER_CHECK_GE(
+              maxAttempts.value(),
+              0,
+              "Invalid configuration: specified 'hive.s3.max-attempts' value {} is < 0.",
+              maxAttempts.value());
           return std::make_shared<Aws::Client::AdaptiveRetryStrategy>(
               maxAttempts.value());
         } else {
@@ -676,9 +683,11 @@ class S3FileSystem::Impl {
         }
       } else if (retryMode.value() == "legacy") {
         if (maxAttempts.has_value()) {
-          VELOX_USER_CHECK(
-              (maxAttempts.value() > 0),
-              "Invalid configuration: specify 'hive.s3.max-attempts' > 0.");
+          VELOX_USER_CHECK_GE(
+              maxAttempts.value(),
+              0,
+              "Invalid configuration: specified 'hive.s3.max-attempts' value {} is < 0.",
+              maxAttempts.value());
           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
@@ -173,7 +173,7 @@ inline std::string getRequestID(
           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'.",         \
+            " Request failed after retrying {} times. Try increasing the value of 'hive.s3.max-attempts'.",                     \
             outcome.GetRetryCount());                                                                                           \
         errMsg.append(retryHint);                                                                                               \
       }                                                                                                                         \