address comments

facebookincubator · May 14, 2024 · 1285c3c · 1285c3c
1 parent 404658a
commit 1285c3c
Show file tree

Hide file tree

Showing 4 changed files with 20 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
@@ -242,7 +242,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
@@ -654,9 +654,11 @@ class S3FileSystem::Impl {
     if (retryMode.has_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 +667,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 +680,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);                                                                                               \
       }                                                                                                                         \