From 1b3716a8c92934b6dc60e1fe3e86b9210bd3bfa7 Mon Sep 17 00:00:00 2001
From: yan ma <yan.ma@intel.com>
Date: Wed, 15 May 2024 03:30:06 +0800
Subject: [PATCH] address comments

---
 velox/connectors/hive/HiveConfig.cpp          |  6 ++---
 velox/connectors/hive/HiveConfig.h            |  2 +-
 .../storage_adapters/s3fs/S3FileSystem.cpp    | 27 ++++++++++++-------
 .../hive/storage_adapters/s3fs/S3Util.h       |  2 +-
 4 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/velox/connectors/hive/HiveConfig.cpp b/velox/connectors/hive/HiveConfig.cpp
index 78f0bb78f0c2a..299fb3c888c63 100644
--- 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
index 8392e3975d632..84f3f5ee00b36 100644
--- 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
index 544c82cd11d5f..cae8657f60e87 100644
--- 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
index 468735d0dfee7..9632dbf47b87e 100644
--- 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);                                                                                               \
       }                                                                                                                         \