pitrou commented on a change in pull request #10877:
URL: https://github.com/apache/arrow/pull/10877#discussion_r685345136
##########
File path: cpp/src/arrow/filesystem/s3fs.h
##########
@@ -69,6 +69,27 @@ enum class S3CredentialsKind : int8_t {
WebIdentity
};
+/// Pure virtual class for describing custom S3 retry strategies
+class S3RetryStrategy {
+ public:
+ /// Simple struct where each field corresponds to a field in
Aws::Client::AWSError
+ struct AWSErrorDetail {
+ /// Corresponds to AWSError::GetErrorType()
+ int error_type;
+ /// Corresponds to AWSError::GetMessage()
+ std::string message;
+ /// Corresponds to AWSError::GetExceptionName()
+ std::string exception_name;
+ /// Corresponds to AWSError::ShouldRetry()
+ bool should_retry;
+ };
+ /// Returns true if the S3 request resulting in the provided error should be
retried.
+ virtual bool ShouldRetry(const AWSErrorDetail& error, int64_t
attempted_retries) = 0;
+ /// Returns the time in miiliseconds the S3 client should sleep for until
retrying.
Review comment:
"milliseconds"
##########
File path: cpp/src/arrow/filesystem/s3fs.cc
##########
@@ -483,6 +484,33 @@ 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
runtime/int
+ S3RetryStrategy::AWSErrorDetail detail = ErrorToDetail(error);
+ return s3_retry_strategy_->ShouldRetry(
+ detail,
+ static_cast<long>(attempted_retries)); // NOLINT runtime/int
Review comment:
You mean `static_cast<int64_t>`?
##########
File path: cpp/src/arrow/filesystem/s3fs_test.cc
##########
@@ -985,6 +986,54 @@ TEST_F(TestS3FS, FileSystemFromUri) {
AssertFileInfo(fs.get(), path, FileType::File, 8);
}
+// Simple retry strategy that records errors encountered and its emitted retry
delays
+class TestRetryStrategy : public S3RetryStrategy {
+ public:
+ virtual ~TestRetryStrategy() = default;
Review comment:
As I wrote, the virtual destructor should probably be defined in the
base class.
##########
File path: cpp/src/arrow/filesystem/s3_internal.h
##########
@@ -178,6 +179,17 @@ inline TimePoint FromAwsDatetime(const
Aws::Utils::DateTime& dt) {
return
std::chrono::time_point_cast<std::chrono::nanoseconds>(dt.UnderlyingTimestamp());
}
+template <typename ErrorType>
+S3RetryStrategy::AWSErrorDetail ErrorToDetail(
+ const Aws::Client::AWSError<ErrorType>& error) {
Review comment:
Since this is useful only in `s3fs.cc`, not in the tests, I would define
this directly inside `WrappedRetryStrategy`.
##########
File path: cpp/src/arrow/filesystem/s3fs.h
##########
@@ -69,6 +69,27 @@ enum class S3CredentialsKind : int8_t {
WebIdentity
};
+/// Pure virtual class for describing custom S3 retry strategies
+class S3RetryStrategy {
+ public:
Review comment:
Shouldn't you add a virtual destructor in this class?
##########
File path: cpp/src/arrow/filesystem/s3fs.cc
##########
@@ -483,6 +484,33 @@ 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
runtime/int
+ S3RetryStrategy::AWSErrorDetail detail = ErrorToDetail(error);
+ return s3_retry_strategy_->ShouldRetry(
+ detail,
+ static_cast<long>(attempted_retries)); // NOLINT runtime/int
+ }
+
+ long CalculateDelayBeforeNextRetry( // NOLINT runtime/int
+ const Aws::Client::AWSError<Aws::Client::CoreErrors>& error,
+ long attempted_retries) const override { // NOLINT runtime/int
+ S3RetryStrategy::AWSErrorDetail detail = ErrorToDetail(error);
+ return static_cast<long>( // NOLINT runtime/int
+ s3_retry_strategy_->CalculateDelayBeforeNextRetry(
+ detail, static_cast<long>(attempted_retries))); // NOLINT
runtime/int
Review comment:
Same here?
--
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]
