pitrou commented on code in PR #13633:
URL: https://github.com/apache/arrow/pull/13633#discussion_r941040935
##########
cpp/src/arrow/filesystem/s3fs.cc:
##########
@@ -209,6 +209,53 @@ bool S3ProxyOptions::Equals(const S3ProxyOptions& other)
const {
username == other.username && password == other.password);
}
+// -----------------------------------------------------------------------
+// AwsRetryStrategy implementation
+
+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) {
Review Comment:
Should mark this `override`.
##########
cpp/src/arrow/filesystem/s3fs.cc:
##########
@@ -209,6 +209,53 @@ bool S3ProxyOptions::Equals(const S3ProxyOptions& other)
const {
username == other.username && password == other.password);
}
+// -----------------------------------------------------------------------
+// AwsRetryStrategy implementation
+
+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,
static_cast<long>(attempted_retries));
+ }
+
+ int64_t CalculateDelayBeforeNextRetry(const AWSErrorDetail& detail,
Review Comment:
Mark this `override`.
##########
python/pyarrow/includes/libarrow_fs.pxd:
##########
@@ -150,6 +150,20 @@ cdef extern from "arrow/filesystem/api.h" namespace
"arrow::fs" nogil:
CS3CredentialsKind_WebIdentity \
"arrow::fs::S3CredentialsKind::WebIdentity"
+ cdef struct CAWSErrorDetail "arrow::fs::S3RetryStrategy::AWSErrorDetail":
Review Comment:
I don't think this needs to be exposed.
##########
python/pyarrow/tests/test_fs.py:
##########
@@ -1112,6 +1110,20 @@ def test_s3_options():
assert pickle.loads(pickle.dumps(fs2)) == fs2
assert fs2 != fs
+ # Check if we have set a C++ AWS Standard retry strategy by testing
+ # if the methods on the strategy quack like a Standard strategy.
+ fs = S3FileSystem(
+ retry_strategy=AwsStandardS3RetryStrategy(max_attempts=5))
+ assert isinstance(fs, S3FileSystem)
+ assert fs.retry_strategy.ShouldRetry(None, 1)
Review Comment:
In any case, I don't think these tests are really needed. Just test that
passing constructor arguments works.
##########
python/pyarrow/tests/test_fs.py:
##########
@@ -15,24 +15,21 @@
# specific language governing permissions and limitations
# under the License.
-from datetime import datetime, timezone, timedelta
import gzip
import os
import pathlib
import pickle
-
-import pytest
import weakref
+from datetime import datetime, timedelta, timezone
Review Comment:
Ideally this would have stayed above ("datetime" is before "gzip").
##########
python/pyarrow/tests/test_fs.py:
##########
@@ -15,24 +15,21 @@
# specific language governing permissions and limitations
# under the License.
-from datetime import datetime, timezone, timedelta
import gzip
import os
import pathlib
import pickle
-
-import pytest
import weakref
+from datetime import datetime, timedelta, timezone
import pyarrow as pa
+import pytest
Review Comment:
Let's not mix PyArrow imports and third-party imports, can you move this
upwards?
##########
python/pyarrow/includes/libarrow_fs.pxd:
##########
@@ -150,6 +150,20 @@ cdef extern from "arrow/filesystem/api.h" namespace
"arrow::fs" nogil:
CS3CredentialsKind_WebIdentity \
"arrow::fs::S3CredentialsKind::WebIdentity"
+ cdef struct CAWSErrorDetail "arrow::fs::S3RetryStrategy::AWSErrorDetail":
+ pass
+
+ cdef cppclass CS3RetryStrategy "arrow::fs::S3RetryStrategy":
+ c_bool ShouldRetry(const CAWSErrorDetail& error, int64_t
attempted_retries)
+ int64_t CalculateDelayBeforeNextRetry(const CAWSErrorDetail& error,
+ int64_t attempted_retries)
Review Comment:
These two methods don't need to be exposed.
##########
python/pyarrow/tests/test_fs.py:
##########
@@ -1112,6 +1110,20 @@ def test_s3_options():
assert pickle.loads(pickle.dumps(fs2)) == fs2
assert fs2 != fs
+ # Check if we have set a C++ AWS Standard retry strategy by testing
+ # if the methods on the strategy quack like a Standard strategy.
+ fs = S3FileSystem(
+ retry_strategy=AwsStandardS3RetryStrategy(max_attempts=5))
+ assert isinstance(fs, S3FileSystem)
+ assert fs.retry_strategy.ShouldRetry(None, 1)
Review Comment:
Hmm... how does this work? `ShouldRetry` is not exposed in Python, is it?
--
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]
