[GitHub] [arrow] lidavidm commented on a diff in pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy

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

lidavidm commented on code in PR #13633:
URL: https://github.com/apache/arrow/pull/13633#discussion_r930979335


##########
cpp/src/arrow/filesystem/s3fs.h:
##########
@@ -92,6 +92,27 @@ class S3RetryStrategy {
                                                 int64_t attempted_retries) = 0;
 };
 
+/// Wrapper for stock AWS retry strategies
+class AwsRetryStrategy : S3RetryStrategy {

Review Comment:
   Does this need to be in the header? For one, we don't want to expose `Aws::` in our public API



##########
python/pyarrow/_s3fs.pyx:
##########
@@ -157,11 +157,14 @@ cdef class S3FileSystem(FileSystem):
                                         'port': 8020, 'username': 'username',
                                         'password': 'password'})
     allow_bucket_creation : bool, default False
-        Whether to allow CreateDir at the bucket-level. This option may also be 
+        Whether to allow CreateDir at the bucket-level. This option may also be
         passed in a URI query parameter.
     allow_bucket_deletion : bool, default False
-        Whether to allow DeleteDir at the bucket-level. This option may also be 
+        Whether to allow DeleteDir at the bucket-level. This option may also be
         passed in a URI query parameter.
+    stock_retry_strategy : str, default None

Review Comment:
   nit: maybe just `retry_strategy`?



##########
python/pyarrow/includes/libarrow_fs.pxd:
##########
@@ -190,6 +194,10 @@ cdef extern from "arrow/filesystem/api.h" namespace "arrow::fs" nogil:
                                   const c_string& external_id,
                                   const int load_frequency)
 
+        @staticmethod
+        shared_ptr[CS3RetryStrategy] GetS3RetryStrategy(const std::string& name,
+                                                             long retry_attempts);

Review Comment:
   ```suggestion
           shared_ptr[CS3RetryStrategy] GetS3RetryStrategy(const std::string& name,
                                                           long retry_attempts);
   ```



##########
cpp/src/arrow/filesystem/s3fs.h:
##########
@@ -92,6 +92,27 @@ class S3RetryStrategy {
                                                 int64_t attempted_retries) = 0;
 };
 
+/// Wrapper for stock AWS retry strategies
+class AwsRetryStrategy : S3RetryStrategy {
+ public:
+  AwsRetryStrategy(const std::shared_ptr<Aws::Client::RetryStrategy>& retry_strategy) {
+    retry_strategy_ = retry_strategy;
+  }
+  ~AwsRetryStrategy() override;
+
+  bool ShouldRetry(const AWSErrorDetail& error, int64_t attempted_retries) {
+    return retry_strategy_.ShouldRetry(error, attempted_retries);

Review Comment:
   I'm not sure an `AWSErrorDetail` will convert into an `AWSError` like this without explicit conversion



-- 
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