Add RetryStrategy for S3 file system #9736

yma11 · 2024-05-07T08:13:29Z

This PR add RetryStrategy support for s3 file system, thus it will retry when connection fails. It also upgrade aws sdk to 1.11.321 which supports AdaptiveRetryStrategy which user may choose to use.

For RetryStrategy, 2 configs are added:

hive.s3.max-attempts: Maximum attempts for connections to a single http client.
hive.s3.retry-mode: 'standard'， 'adaptive' or legacy, client will be created w/o retrystrategy if it's empty.

netlify · 2024-05-07T08:13:45Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`e927721`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/6666ac033cbfe600090421b5

yma11 · 2024-05-07T08:20:01Z

@majetideepak Can you help review this PR? cc @FelixYBW

majetideepak

@yma11 two minor comments. Thanks for this change!

majetideepak · 2024-05-07T12:09:12Z

velox/connectors/hive/storage_adapters/s3fs/S3FileSystem.cpp

@@ -640,6 +644,39 @@ class S3FileSystem::Impl {
    return getDefaultCredentialsProvider();
  }

+  // Return a client RetryStrategy based on the config.
+  std::shared_ptr<Aws::Client::RetryStrategy> getRetryStrategy() const {


We can make getRetryStrategy() return std::optional<std::shared_ptr<Aws::Client::RetryStrategy>> and use the library default.

Do you mean keeping the choice that create the client without any RetryStrategy?

Updated a little. Please take a look.

majetideepak · 2024-05-07T12:09:49Z

velox/docs/configs.rst

+   * - hive.s3.retry-mode
+     - string
+     -
+     - 'standard' or 'adaptive', use DefaultRetryStrategy if it's empty.


Can you add some description here on what standard and adaptive retry strategies are? Thanks!

FelixYBW · 2024-05-07T16:43:38Z

velox/connectors/hive/storage_adapters/s3fs/S3FileSystem.cpp

+          // Otherwise, use default value 3.
+          return std::make_shared<Aws::Client::AdaptiveRetryStrategy>();
+        }
+      }


Can you add the check of invalid value of retry mode?

FelixYBW · 2024-05-07T16:45:07Z

velox/docs/configs.rst

@@ -536,7 +536,16 @@ Each query can override the config by setting corresponding query session proper
     - integer
     -
     - Maximum concurrent TCP connections for a single http client.
-
+   * - hive.s3.max-attempts


By default, aws sdk uses 3, right? Let's mention this here

FelixYBW · 2024-05-08T03:12:50Z

@yma11 When the call client_->HeadObject or client_->GetObject fail, can we get the the retry number from outcome? If so, let's print the number in error message. So user can know it does be retried and may increase retry number.

majetideepak

@yma11 I will approve again once @FelixYBW comment is addressed.

yma11 · 2024-05-08T07:12:23Z

velox/connectors/hive/storage_adapters/s3fs/S3Util.h

+      if (IsRetryableHttpResponseCode(error.GetResponseCode())) {                                                               \
+        auto retryHint = fmt::format(                                                                                           \
+            "This request has retried {} times, you may can try increasing 'hive.s3.max-attempts'.",                            \
+            outcome.GetRetryCount());                                                                                           \
+        errMsg.append(retryHint);                                                                                               \


@FelixYBW Please review this change.

jinchengchenghh · 2024-05-08T07:38:54Z

velox/docs/configs.rst

+   * - hive.s3.max-attempts
+     - integer
+     -
+     - Maximum attempts for connections to a single http client, work together with retry-mode. By default, it's 3 for standard/adaptive mode


As this document describe, default 4 for legacy mode

A default value of 4 for maximum retry attempts,

https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-retries.html

Get this number from code, maybe more reliable?

their cpp_sdk and document have some difference. Let's follow sdk

Maybe different AWS version has different default value

jinchengchenghh · 2024-05-08T07:39:32Z

velox/connectors/hive/HiveConfig.cpp

@@ -120,6 +120,16 @@ std::optional<uint32_t> HiveConfig::s3MaxConnections() const {
      config_->get<uint32_t>(kS3MaxConnections));
 }

+std::optional<uint32_t> HiveConfig::s3MaxAttempts() const {


does it is int32_t in AWS config?

yma11 · 2024-05-08T12:30:16Z

@yma11 I will approve again once @FelixYBW comment is addressed.

Thanks @majetideepak, I have updated. Can you take a look again? There is a build failure in CI, which is caused by the old version aws sdk doesn't have GetRetryCount() API and seems version upgrade in this PR can't be reflected immediately.

majetideepak · 2024-05-08T16:05:28Z

There is a build failure in CI, which is caused by the old version aws sdk doesn't have GetRetryCount() API

@yma11 Let's update the library first in a separate PR (changes inside setup-adapters.sh). That will give us the newer CI images with the updated library.

yma11 · 2024-05-09T01:26:17Z

There is a build failure in CI, which is caused by the old version aws sdk doesn't have GetRetryCount() API

@yma11 Let's update the library first in a separate PR (changes inside setup-adapters.sh). That will give us the newer CI images with the updated library.

Thanks for suggestion. I have created 9756 for version upgrade. Will update this PR once it's merged.

yma11 · 2024-05-14T00:22:10Z

@majetideepak can you approve this PR again? Thanks.

majetideepak

@yma11 some minor comments.
Were you able to test this change on your local setup?

majetideepak · 2024-05-14T02:41:01Z

velox/connectors/hive/storage_adapters/s3fs/S3FileSystem.cpp

+        if (maxAttempts.has_value()) {
+          VELOX_USER_CHECK(
+              (maxAttempts.value() > 0),
+              "Invalid configuration: specify 'hive.s3.max-attempts' > 0.");


Maybe print the value?
Should 0 be allowed here?
VELOX_USER_CHECK( (maxAttempts.value() >= 0), "Invalid configuration: specified 'hive.s3.max-attempts' value {} is < 0.", maxAttempts.value());

As checked in CilentConfiguration, it can be 0. Changed it to use VELOX_USER_CHECK_GE. @jinchengchenghh

majetideepak · 2024-05-14T02:41:24Z

velox/connectors/hive/storage_adapters/s3fs/S3FileSystem.cpp

+      } else if (retryMode.value() == "adaptive") {
+        if (maxAttempts.has_value()) {
+          VELOX_USER_CHECK(
+              (maxAttempts.value() > 0),


same as above. Should 0 be allowed and specify the value in the error message.

majetideepak · 2024-05-14T02:41:45Z

velox/connectors/hive/storage_adapters/s3fs/S3FileSystem.cpp

+      } else if (retryMode.value() == "legacy") {
+        if (maxAttempts.has_value()) {
+          VELOX_USER_CHECK(
+              (maxAttempts.value() > 0),


same as above. Should 0 be allowed and specify the value in the error message.

majetideepak · 2024-05-14T02:46:03Z

velox/connectors/hive/storage_adapters/s3fs/S3Util.h

+          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'.",         \


nit: Request failed after retrying {} times. Try increasing the value of 'hive.s3.max-attempts'.

jinchengchenghh · 2024-05-14T02:48:28Z

velox/connectors/hive/storage_adapters/s3fs/S3FileSystem.cpp

+    if (retryMode.has_value()) {
+      if (retryMode.value() == "standard") {
+        if (maxAttempts.has_value()) {
+          VELOX_USER_CHECK(


VELOX_USER_CHECK -> VELOX_USER_CHECK_GT

jinchengchenghh · 2024-05-14T02:48:39Z

velox/connectors/hive/storage_adapters/s3fs/S3FileSystem.cpp

+        }
+      } else if (retryMode.value() == "adaptive") {
+        if (maxAttempts.has_value()) {
+          VELOX_USER_CHECK(


jinchengchenghh · 2024-05-14T02:49:03Z

velox/connectors/hive/storage_adapters/s3fs/S3FileSystem.cpp

+        }
+      } else if (retryMode.value() == "legacy") {
+        if (maxAttempts.has_value()) {
+          VELOX_USER_CHECK(


yma11 · 2024-05-14T11:56:40Z

@yma11 some minor comments. Were you able to test this change on your local setup?

I don't have local env for this test for now. I will add corresponding configs at Gluten side and then test it in AWS.

majetideepak · 2024-05-14T16:40:30Z

I don't have local env for this test for now. I will add corresponding configs at Gluten side and then test it in AWS.

@yma11 thanks! Can you please confirm and update here? We cannot add CI tests for some of these changes, but the expectation is that the author tests the changes on their end.

FelixYBW · 2024-05-18T06:11:49Z

velox/connectors/hive/storage_adapters/s3fs/S3FileSystem.cpp

+      const {
+    auto retryMode = hiveConfig_->s3RetryMode();
+    auto maxAttempts = hiveConfig_->s3MaxAttempts();
+    if (retryMode.has_value()) {


If user doesn't config retryMode but config maxAttempts, does maxAttempts take effect? It should takes effect since we have default retry mode.

No. In velox, maxAttempts need work together with retryMode. If retryMode isn't configured, S3 client will be created w/o RetryStrategy. But for Gluten, we've set default value for retryMode. I updated the doc in this PR.

FelixYBW · 2024-05-25T02:17:03Z

@yma11 thanks! Can you please confirm and update here? We cannot add CI tests for some of these changes, but the expectation is that the author tests the changes on their end.

Confirmed it works. Retry number is passed to S3 and the query passed.

yma11 · 2024-06-03T01:13:54Z

@majetideepak can you help ping anyone who can merge this PR? Thanks.

majetideepak · 2024-06-03T09:08:51Z

@pedroerp can you please help merge this? Thanks!

majetideepak · 2024-06-10T04:28:25Z

@yma11 I pinged Pedro again. Can you please rebase with main? Sorry for the delay.

yma11 · 2024-06-10T10:03:22Z

@yma11 I pinged Pedro again. Can you please rebase with main? Sorry for the delay.

Done. Really thanks for driving merge.

facebook-github-bot · 2024-06-10T17:30:30Z

@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-06-15T10:44:58Z

@kgpai merged this pull request in 8c3630b.

conbench-facebook · 2024-06-15T11:09:20Z

Conbench analyzed the 1 benchmark run on commit 8c3630b3.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 7, 2024

majetideepak reviewed May 7, 2024

View reviewed changes

yma11 force-pushed the s3-retry branch from d2a0f4b to 25d3085 Compare May 7, 2024 13:53

FelixYBW reviewed May 7, 2024

View reviewed changes

yma11 force-pushed the s3-retry branch from 25d3085 to 6f156cb Compare May 8, 2024 00:46

majetideepak approved these changes May 8, 2024

View reviewed changes

majetideepak added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label May 8, 2024

majetideepak removed the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label May 8, 2024

majetideepak requested changes May 8, 2024

View reviewed changes

yma11 force-pushed the s3-retry branch from ce52cbf to 2705585 Compare May 8, 2024 07:11

yma11 commented May 8, 2024

View reviewed changes

jinchengchenghh reviewed May 8, 2024

View reviewed changes

yma11 force-pushed the s3-retry branch 3 times, most recently from f052a02 to c2f4d18 Compare May 8, 2024 10:39

yma11 force-pushed the s3-retry branch 2 times, most recently from 6360d25 to 00bc6ad Compare May 11, 2024 01:30

yma11 requested a review from majetideepak May 13, 2024 01:16

majetideepak reviewed May 14, 2024

View reviewed changes

jinchengchenghh reviewed May 14, 2024

View reviewed changes

yma11 force-pushed the s3-retry branch from 00bc6ad to 8eedc3d Compare May 14, 2024 11:55

yma11 force-pushed the s3-retry branch from 8eedc3d to 1285c3c Compare May 14, 2024 15:30

FelixYBW reviewed May 18, 2024

View reviewed changes

yma11 force-pushed the s3-retry branch from 1285c3c to d0bb3b0 Compare May 20, 2024 02:10

majetideepak approved these changes May 25, 2024

View reviewed changes

majetideepak added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label May 25, 2024

yma11 force-pushed the s3-retry branch from d0bb3b0 to accfb93 Compare June 2, 2024 03:20

yma11 force-pushed the s3-retry branch from accfb93 to e927721 Compare June 10, 2024 07:32

yma11 added 4 commits June 10, 2024 23:07

Add RetryStrategy for S3 file system

ee9a09f

address comments

cc1f73c

Add hint based on response code and retrycount

0e1ac70

address comments

e927721

facebook-github-bot closed this in 8c3630b Jun 15, 2024

facebook-github-bot added the Merged label Jun 15, 2024

Add RetryStrategy for S3 file system #9736

Add RetryStrategy for S3 file system #9736

Conversation

yma11 commented May 7, 2024 • edited Loading

netlify bot commented May 7, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

yma11 commented May 7, 2024

majetideepak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yma11 May 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FelixYBW commented May 8, 2024

majetideepak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yma11 commented May 8, 2024

majetideepak commented May 8, 2024 • edited Loading

yma11 commented May 9, 2024

yma11 commented May 14, 2024

majetideepak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yma11 commented May 14, 2024

majetideepak commented May 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FelixYBW commented May 25, 2024 • edited Loading

yma11 commented Jun 3, 2024

majetideepak commented Jun 3, 2024

majetideepak commented Jun 10, 2024

yma11 commented Jun 10, 2024

facebook-github-bot commented Jun 10, 2024

facebook-github-bot commented Jun 15, 2024

conbench-facebook bot commented Jun 15, 2024

yma11 commented May 7, 2024 •

edited

Loading

netlify bot commented May 7, 2024 •

edited

Loading

yma11 May 7, 2024 •

edited

Loading

majetideepak commented May 8, 2024 •

edited

Loading

majetideepak commented May 14, 2024 •

edited

Loading

FelixYBW commented May 25, 2024 •

edited

Loading