[GitHub] [arrow] lidavidm commented on a change in pull request #10877: ARROW-13508: [C++] Support custom retry strategies in S3Options

[GitBox <gi...@apache.org>]

lidavidm commented on a change in pull request #10877:
URL: https://github.com/apache/arrow/pull/10877#discussion_r682888810



##########
File path: cpp/src/arrow/filesystem/s3fs.cc
##########
@@ -483,6 +483,30 @@ std::string FormatRange(int64_t start, int64_t length) {
   return ss.str();
 }
 
+// An AWS RetryStrategy that wraps a provided arrow::fs::S3RetryStrategy
+class WrappedRetryStrategy : public Aws::Client::RetryStrategy {
+ public:
+  explicit WrappedRetryStrategy(const std::shared_ptr<S3RetryStrategy>& s3_retry_strategy)
+      : s3_retry_strategy_(s3_retry_strategy) {}
+
+  bool ShouldRetry(const Aws::Client::AWSError<Aws::Client::CoreErrors>& error,
+                   long attempted_retries) const override {  // NOLINT

Review comment:
       sorry, what's the NOLINT for? Nothing stands out to me as violating standards 

##########
File path: cpp/src/arrow/filesystem/s3fs.h
##########
@@ -69,6 +69,13 @@ enum class S3CredentialsKind : int8_t {
   WebIdentity
 };
 
+/// Pure virtual class for describing custom S3 retry strategies
+class S3RetryStrategy {
+ public:
+  virtual bool ShouldRetry(Status& error, long attempted_retries) = 0;
+  virtual long CalculateDelayBeforeNextRetry(Status& error, long attempted_retries) = 0;

Review comment:
       That could work, and/or it could be provided via a StatusDetail though that may not be as simple. https://github.com/apache/arrow/blob/72b52ef1b2681f927e8cfc2bc6a643648c542058/cpp/src/arrow/status.h#L99-L113




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org