lidavidm commented on code in PR #13633:
URL: https://github.com/apache/arrow/pull/13633#discussion_r933543387
##########
python/pyarrow/_s3fs.pyx:
##########
@@ -157,11 +157,16 @@ 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.
+ retry_strategy : str, default None
+ The name of the retry strategy to use with S3. Valid values are
+ "aws_standard" and "aws_default". Ignore with None.
Review Comment:
Is there a name for the legacy retry strategy, so that instead of "Ignore
with None", we can say what it actually defaults to?
##########
cpp/src/arrow/filesystem/s3fs.cc:
##########
@@ -380,6 +411,23 @@ Result<S3Options> S3Options::FromUri(const std::string&
uri_string,
return FromUri(uri, out_path);
}
+std::shared_ptr<S3RetryStrategy> GetS3RetryStrategy(const std::string& name,
+ long retry_attempts) {
+ if (name == "aws_standard") {
+ auto aws_standard = std::make_shared<AwsRetryStrategy>(
+ std::make_shared<Aws::Client::StandardRetryStrategy>(retry_attempts));
+ return std::static_pointer_cast<S3RetryStrategy>(aws_standard);
+ }
+
+ if (name == "aws_default") {
+ auto aws_default = std::make_shared<AwsRetryStrategy>(
+ std::make_shared<Aws::Client::DefaultRetryStrategy>(retry_attempts));
+ return std::static_pointer_cast<S3RetryStrategy>(aws_default);
+ }
+
+ return NULL;
Review Comment:
Use `nullptr` in C++ code
Though: I would almost rather have this return
`Result<std::shared_ptr<S3RetryStrategy>>` instead so that we can have an
actual error if an invalid value is passed.
##########
cpp/src/arrow/filesystem/s3fs.cc:
##########
@@ -209,6 +209,37 @@ bool S3ProxyOptions::Equals(const S3ProxyOptions& other)
const {
username == other.username && password == other.password);
}
+// A wrapper to allow us to supply an Aws::Client::RetryStrategy as an
S3RetryStrategy
+class AwsRetryStrategy : public S3RetryStrategy {
+ public:
+ AwsRetryStrategy(const std::shared_ptr<Aws::Client::RetryStrategy>&
retry_strategy) {
+ retry_strategy_ = retry_strategy;
+ }
Review Comment:
```suggestion
AwsRetryStrategy(const std::shared_ptr<Aws::Client::RetryStrategy>&
retry_strategy) : retry_strategy_(retry_strategy) {}
```
##########
python/pyarrow/_s3fs.pyx:
##########
@@ -157,11 +157,16 @@ 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.
+ retry_strategy : str, default None
+ The name of the retry strategy to use with S3. Valid values are
+ "aws_standard" and "aws_default". Ignore with None.
Review Comment:
Or otherwise maybe just word as "If not given, uses a default retry
strategy" or something similar (don't know if it's worth describing the
'legacy' retry strategy)
##########
cpp/src/arrow/filesystem/s3fs.cc:
##########
@@ -209,6 +209,37 @@ bool S3ProxyOptions::Equals(const S3ProxyOptions& other)
const {
username == other.username && password == other.password);
}
+// A wrapper to allow us to supply an Aws::Client::RetryStrategy as an
S3RetryStrategy
+class AwsRetryStrategy : public S3RetryStrategy {
+ public:
+ AwsRetryStrategy(const std::shared_ptr<Aws::Client::RetryStrategy>&
retry_strategy) {
+ retry_strategy_ = retry_strategy;
+ }
+
+ bool ShouldRetry(const AWSErrorDetail& detail, int64_t attempted_retries) {
+ Aws::Client::AWSError<Aws::Client::CoreErrors> error =
DetailToError(detail);
+ return retry_strategy_->ShouldRetry(error, (long)attempted_retries);
Review Comment:
```suggestion
return retry_strategy_->ShouldRetry(error,
static_cast<long>(attempted_retries));
```
##########
cpp/src/arrow/filesystem/s3fs.cc:
##########
@@ -209,6 +209,37 @@ bool S3ProxyOptions::Equals(const S3ProxyOptions& other)
const {
username == other.username && password == other.password);
}
+// A wrapper to allow us to supply an Aws::Client::RetryStrategy as an
S3RetryStrategy
+class AwsRetryStrategy : public S3RetryStrategy {
+ public:
+ AwsRetryStrategy(const std::shared_ptr<Aws::Client::RetryStrategy>&
retry_strategy) {
+ retry_strategy_ = retry_strategy;
+ }
+
+ bool ShouldRetry(const AWSErrorDetail& detail, int64_t attempted_retries) {
+ Aws::Client::AWSError<Aws::Client::CoreErrors> error =
DetailToError(detail);
+ return retry_strategy_->ShouldRetry(error, (long)attempted_retries);
+ }
+
+ int64_t CalculateDelayBeforeNextRetry(const AWSErrorDetail& detail,
+ int64_t attempted_retries) {
+ Aws::Client::AWSError<Aws::Client::CoreErrors> error =
DetailToError(detail);
+ return retry_strategy_->CalculateDelayBeforeNextRetry(error,
(long)attempted_retries);
+ }
+
+ private:
+ std::shared_ptr<Aws::Client::RetryStrategy> retry_strategy_;
+ static Aws::Client::AWSError<Aws::Client::CoreErrors> DetailToError(
+ const S3RetryStrategy::AWSErrorDetail& detail) {
+ auto exception_name = new Aws::String(detail.exception_name.c_str());
+ auto message = new Aws::String(detail.message.c_str());
+ auto errors = new Aws::Client::AWSError<Aws::Client::CoreErrors>(
+ static_cast<Aws::Client::CoreErrors>(detail.error_type),
*exception_name,
+ *message, detail.should_retry);
+ return *errors;
Review Comment:
```suggestion
auto errors = Aws::Client::AWSError<Aws::Client::CoreErrors>(
static_cast<Aws::Client::CoreErrors>(detail.error_type),
exception_name,
message, detail.should_retry);
return errors;
```
##########
cpp/src/arrow/filesystem/s3fs.cc:
##########
@@ -209,6 +209,37 @@ bool S3ProxyOptions::Equals(const S3ProxyOptions& other)
const {
username == other.username && password == other.password);
}
+// A wrapper to allow us to supply an Aws::Client::RetryStrategy as an
S3RetryStrategy
+class AwsRetryStrategy : public S3RetryStrategy {
+ public:
+ AwsRetryStrategy(const std::shared_ptr<Aws::Client::RetryStrategy>&
retry_strategy) {
+ retry_strategy_ = retry_strategy;
+ }
+
+ bool ShouldRetry(const AWSErrorDetail& detail, int64_t attempted_retries) {
+ Aws::Client::AWSError<Aws::Client::CoreErrors> error =
DetailToError(detail);
+ return retry_strategy_->ShouldRetry(error, (long)attempted_retries);
+ }
+
+ int64_t CalculateDelayBeforeNextRetry(const AWSErrorDetail& detail,
+ int64_t attempted_retries) {
+ Aws::Client::AWSError<Aws::Client::CoreErrors> error =
DetailToError(detail);
+ return retry_strategy_->CalculateDelayBeforeNextRetry(error,
(long)attempted_retries);
+ }
+
+ private:
+ std::shared_ptr<Aws::Client::RetryStrategy> retry_strategy_;
+ static Aws::Client::AWSError<Aws::Client::CoreErrors> DetailToError(
+ const S3RetryStrategy::AWSErrorDetail& detail) {
+ auto exception_name = new Aws::String(detail.exception_name.c_str());
+ auto message = new Aws::String(detail.message.c_str());
Review Comment:
```suggestion
auto exception_name = ToAwsString(detail.exception_name);
auto message = ToAwsString(detail.message);
```
##########
cpp/src/arrow/filesystem/s3fs.cc:
##########
@@ -209,6 +209,37 @@ bool S3ProxyOptions::Equals(const S3ProxyOptions& other)
const {
username == other.username && password == other.password);
}
+// A wrapper to allow us to supply an Aws::Client::RetryStrategy as an
S3RetryStrategy
+class AwsRetryStrategy : public S3RetryStrategy {
+ public:
+ AwsRetryStrategy(const std::shared_ptr<Aws::Client::RetryStrategy>&
retry_strategy) {
+ retry_strategy_ = retry_strategy;
+ }
+
+ bool ShouldRetry(const AWSErrorDetail& detail, int64_t attempted_retries) {
+ Aws::Client::AWSError<Aws::Client::CoreErrors> error =
DetailToError(detail);
+ return retry_strategy_->ShouldRetry(error, (long)attempted_retries);
+ }
+
+ int64_t CalculateDelayBeforeNextRetry(const AWSErrorDetail& detail,
+ int64_t attempted_retries) {
+ Aws::Client::AWSError<Aws::Client::CoreErrors> error =
DetailToError(detail);
+ return retry_strategy_->CalculateDelayBeforeNextRetry(error,
(long)attempted_retries);
Review Comment:
```suggestion
return retry_strategy_->CalculateDelayBeforeNextRetry(error,
static_cast<long>(attempted_retries));
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
