You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/07/18 11:29:40 UTC
[GitHub] [arrow] 3dbrows opened a new pull request, #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
3dbrows opened a new pull request, #13633:
URL: https://github.com/apache/arrow/pull/13633
https://issues.apache.org/jira/browse/ARROW-17057
--
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
[GitHub] [arrow] lidavidm commented on a diff in pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by 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
[GitHub] [arrow] pitrou commented on pull request #13633: ARROW-17057: [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
pitrou commented on PR #13633:
URL: https://github.com/apache/arrow/pull/13633#issuecomment-1209187870
I seems the test now actually fails? (at least [on AppVeyor](https://ci.appveyor.com/project/ApacheSoftwareFoundation/arrow/builds/44414052/job/t4j75ir56dhui6u2#L2818))
```
================================== FAILURES ===================================
_______________________________ test_s3_options _______________________________
@pytest.mark.s3
def test_s3_options():
> from pyarrow.fs import (AwsDefaultS3RetryStrategy,
AwsStandardS3RetryStrategy, S3FileSystem,
S3RetryStrategy)
E ImportError: cannot import name 'AwsDefaultS3RetryStrategy' from 'pyarrow.fs' (c:\projects\arrow\python\pyarrow\fs.py)
```
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #13633:
URL: https://github.com/apache/arrow/pull/13633#issuecomment-1187219195
https://issues.apache.org/jira/browse/ARROW-17057
--
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
[GitHub] [arrow] wjones127 commented on pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
wjones127 commented on PR #13633:
URL: https://github.com/apache/arrow/pull/13633#issuecomment-1187623154
It looks like `InitRetryStrategy` was added 7 months ago and looks like the package managers we depend on haven't all caught up. Perhaps there is a simple version of the implementation we can include? https://github.com/aws/aws-sdk-cpp/blob/2ac79af8c551aff94ef5d28c88bf09209b0b71c8/aws-cpp-sdk-core/source/client/ClientConfiguration.cpp#L189
--
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
[GitHub] [arrow] pitrou commented on a diff in pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #13633:
URL: https://github.com/apache/arrow/pull/13633#discussion_r934598841
##########
cpp/src/arrow/filesystem/s3fs.h:
##########
@@ -152,6 +152,10 @@ struct ARROW_EXPORT S3Options {
S3Options();
+ /// Get a stock AWS retry strategy from a string.
+ static std::shared_ptr<S3RetryStrategy> GetS3RetryStrategy(const std::string& name,
Review Comment:
This should probably be a static method of `S3RetryStrategy` instead. Also, the `name` parameter should be explained in the docstring.
##########
python/pyarrow/includes/libarrow_fs.pxd:
##########
@@ -164,6 +167,8 @@ cdef extern from "arrow/filesystem/api.h" namespace "arrow::fs" nogil:
int load_frequency
CS3ProxyOptions proxy_options
CS3CredentialsKind credentials_kind
+ shared_ptr[CS3RetryStrategy] retry_strategy
+ int retry_max_attempts
Review Comment:
`int retry_max_attempts` doesn't seem to actually exist?
##########
cpp/src/arrow/filesystem/s3fs.h:
##########
@@ -152,6 +152,10 @@ struct ARROW_EXPORT S3Options {
S3Options();
+ /// Get a stock AWS retry strategy from a string.
+ static std::shared_ptr<S3RetryStrategy> GetS3RetryStrategy(const std::string& name,
+ long retry_attempts);
Review Comment:
Can you make this `int64_t max_attempts`? (i.e. use `int64_t` instead of `long`, and name the argument better)
##########
cpp/src/arrow/filesystem/s3fs.h:
##########
@@ -152,6 +152,10 @@ struct ARROW_EXPORT S3Options {
S3Options();
+ /// Get a stock AWS retry strategy from a string.
+ static std::shared_ptr<S3RetryStrategy> GetS3RetryStrategy(const std::string& name,
Review Comment:
Also, instead of passing a made-up name, I would favour two separate methods `GetAwsDefaultRetryStrategy` and `GetAwsStandardRetryStrategy`.
##########
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);
Review Comment:
I don't think the `static_pointer_cast` is required since implicit conversion should be available.
##########
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.
+ retry_max_attempts : int, default 3
+ The maximum number of attempts to pass to AWS retry strategies.
Review Comment:
I think it is a bad design choice to use two separate arguments for this.
I would favour a single argument, perhaps together with more structured objects, e.g.:
```python
class AwsDefaultRetryStrategy(object):
def __init__(self, max_attempts=3):
self.max_attempts = max_attempts
```
##########
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:
Talking about "default" here would introduce confusion with "aws_default" (the AWS SDK's fault for poor naming, between "standard", "default" and "legacy"...)
--
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
[GitHub] [arrow] 3dbrows commented on pull request #13633: ARROW-17057: [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
3dbrows commented on PR #13633:
URL: https://github.com/apache/arrow/pull/13633#issuecomment-1209200271
🤔 Maybe I need to update this...
https://github.com/apache/arrow/blob/3148884ed74a824ac6acc341fa50baef45c46669/python/pyarrow/fs.py#L53-L56
--
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
[GitHub] [arrow] lidavidm commented on pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #13633:
URL: https://github.com/apache/arrow/pull/13633#issuecomment-1208212853
Even with that, we'd need a `static Deserialize` at which point we're stuck if we also want to enable user-defined retry strategies, unless we go all the way to having a registry.
--
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
[GitHub] [arrow] pitrou commented on a diff in pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #13633:
URL: https://github.com/apache/arrow/pull/13633#discussion_r941108741
##########
python/pyarrow/tests/test_fs.py:
##########
@@ -1107,6 +1107,10 @@ def test_s3_options():
assert isinstance(fs, S3FileSystem)
assert pickle.loads(pickle.dumps(fs)) == fs
+ fs = S3FileSystem(
+ retry_strategy=AwsStandardS3RetryStrategy(max_attempts=5))
+ assert isinstance(fs, S3FileSystem)
Review Comment:
Also test `AwsDefaultRetryStrategy`?
--
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
[GitHub] [arrow] 3dbrows commented on pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
3dbrows commented on PR #13633:
URL: https://github.com/apache/arrow/pull/13633#issuecomment-1196876151
Thanks @lidavidm . I've responded to your comments. Still testing to be done, and I'm not sure `S3Options` is really the place to have the "get strategy" function.
--
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
[GitHub] [arrow] lidavidm commented on pull request #13633: ARROW-17057: [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #13633:
URL: https://github.com/apache/arrow/pull/13633#issuecomment-1210679434
Thanks for sticking through this @3dbrows!
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #13633:
URL: https://github.com/apache/arrow/pull/13633#issuecomment-1187219252
:warning: Ticket **has not been started in JIRA**, please click 'Start Progress'.
--
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
[GitHub] [arrow] 3dbrows commented on pull request #13633: ARROW-17057: [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
3dbrows commented on PR #13633:
URL: https://github.com/apache/arrow/pull/13633#issuecomment-1210683059
Thanks @lidavidm @pitrou @wjones127 for your patience 😄
--
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
[GitHub] [arrow] 3dbrows commented on pull request #13633: ARROW-17057: [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
3dbrows commented on PR #13633:
URL: https://github.com/apache/arrow/pull/13633#issuecomment-1210312379
@pitrou , now that all checks are passing, I assume [the merge procedure](https://github.com/apache/arrow/tree/master/dev#how-to-merge-a-pull-request) has to be done by a maintainer?
Should I now move the JIRA ticket to another state?
--
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
[GitHub] [arrow] 3dbrows commented on pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
3dbrows commented on PR #13633:
URL: https://github.com/apache/arrow/pull/13633#issuecomment-1192622408
I will need some help with getting this over the line; hopefully my intention is clear from the test comments but as a C++ novice I am not sure how to parse some of the compiler errors that result from building the tests.
In principle it's quite simple: the intention is that you can optionally supply an enum to `S3Options` to select a stock AWS retry strategy.
--
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
[GitHub] [arrow] lidavidm commented on pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #13633:
URL: https://github.com/apache/arrow/pull/13633#issuecomment-1194428125
Ah, this is what I get for not reading fully. Will's proposal seems basically fine to me.
--
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
[GitHub] [arrow] 3dbrows commented on pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
3dbrows commented on PR #13633:
URL: https://github.com/apache/arrow/pull/13633#issuecomment-1187151257
@github-actions autotune
--
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
[GitHub] [arrow] 3dbrows commented on pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
3dbrows commented on PR #13633:
URL: https://github.com/apache/arrow/pull/13633#issuecomment-1195701184
Thanks guys. I've updated the code with some illustrative (not final) code of what I think the idea is. Can you confirm this is what you meant?
What I'm driving at is that we have a subclass of `S3RetryStrategy` called `AwsRetryStrategy` which takes a constructor argument of a stock AWS strategy, itself having been created by `GetS3RetryStrategy()`. (It might be better to have this live elsewhere.)
We could of course broaden `GetS3RetryStrategy()` to take the name of any strategy, not just an AWS one. You could pass in a string like `aws_standard` or `arrow_connect` or whatever. (Or enum, even better.) After all, it ultimately just returns you an `S3RetryStrategy`.
--
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
[GitHub] [arrow] lidavidm commented on pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #13633:
URL: https://github.com/apache/arrow/pull/13633#issuecomment-1196877923
Thanks for bearing with us. I kicked off CI and I'll take another look soon.
--
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
[GitHub] [arrow] lidavidm commented on a diff in pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #13633:
URL: https://github.com/apache/arrow/pull/13633#discussion_r939153193
##########
python/pyarrow/_s3fs.pyx:
##########
@@ -300,6 +327,14 @@ cdef class S3FileSystem(FileSystem):
options.allow_bucket_creation = allow_bucket_creation
options.allow_bucket_deletion = allow_bucket_deletion
+ self._retry_strategy = retry_strategy
+ if isinstance(retry_strategy, AwsStandardRetryStrategy):
+ options.retry_strategy = CS3RetryStrategy.GetAwsStandardRetryStrategy(retry_strategy.max_attempts)
+ elif isinstance(retry_strategy, AwsDefaultRetryStrategy):
+ options.retry_strategy = CS3RetryStrategy.GetAwsDefaultRetryStrategy(retry_strategy.max_attempts)
+ else:
+ raise ValueError('Invalid retry_strategy.')
Review Comment:
```suggestion
raise ValueError(f'Invalid retry_strategy {retry_strategy!r}')
```
##########
python/pyarrow/_s3fs.pyx:
##########
@@ -87,6 +87,29 @@ def resolve_s3_region(bucket):
return frombytes(c_region)
+class AwsRetryStrategy(object):
Review Comment:
Maybe 'S3RetryStrategy' to be in line with the C++ side? And then 'AwsStandardS3RetryStrategy'
##########
python/pyarrow/_s3fs.pyx:
##########
@@ -87,6 +87,29 @@ def resolve_s3_region(bucket):
return frombytes(c_region)
+class AwsRetryStrategy(object):
Review Comment:
```suggestion
class AwsRetryStrategy:
```
--
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
[GitHub] [arrow] lidavidm merged pull request #13633: ARROW-17057: [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
lidavidm merged PR #13633:
URL: https://github.com/apache/arrow/pull/13633
--
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
[GitHub] [arrow] wjones127 commented on pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
wjones127 commented on PR #13633:
URL: https://github.com/apache/arrow/pull/13633#issuecomment-1194196307
Hmm I think I may have led you astray. Sorry about that.
> My confusion is that it feels like attempting duck-typing in a type-safe language, which I didn't think would be something you could do.
I tested this and your intuition is correct. Odd though that we don't provide a way to wrap an `Aws::Client::RetryStrategy` in a `arrow::fs::AwsRetryStrategy`. Maybe it's just that no one has requested that. @pitrou @lidavidm any thoughts on that?
Having looked a little more at boto3, I think we probably want to support two parameters: `retry_mode` and `retry_attempts`, corresponding to the boto3 arguments here:
https://boto3.amazonaws.com/v1/documentation/api/latest/guide/retries.html
Then we'll need a C++ function:
```cpp
arrow::fs::AwsRetryStrategy GetS3RetryStrategy(const std::string& name, long retry_attempts) {
...
}
```
You should then be able to expose that function in Cython.
One issue: it does look like which retry strategies are available depends on the version of the AwsSdk. Adaptive isn't available in the one currently installed on my computer.
--
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
[GitHub] [arrow] 3dbrows commented on pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
3dbrows commented on PR #13633:
URL: https://github.com/apache/arrow/pull/13633#issuecomment-1193827299
Had another go at this, although have some doubts.
* Cython imports: I'm importing what I think are the very minimally necessary definitions from the C++ headers. In particular,
* I need nothing from `Aws::Client::RetryStrategy` except to know that it exists;
* I need to know only the constructors of `Aws::Client::StandardRetryStrategy` and `Aws::Client::AdaptiveRetryStrategy` so I can reference them elsewhere;
* My line of thinking is that this gives me all I need for Cython to `make_shared` the underlying C++ types, but this is quite possibly wrongheaded (I haven't touched Cython before).
* Typing questions
* The `S3Options` type takes an `S3RetryStrategy`, but in Cython we're now feeding it a subclass of `Aws::Client::RetryStrategy` - which so happens to have similar method signatures. Whether this is considered "safe" or "correct" (or even "works") I am not sure. My confusion is that it feels like attempting duck-typing in a type-safe language, which I didn't think would be something you could do.
--
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
[GitHub] [arrow] pitrou commented on pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
pitrou commented on PR #13633:
URL: https://github.com/apache/arrow/pull/13633#issuecomment-1208210324
> I'm not sure how to handle passing the strategy in the `__reduce__` method. A bit stuck there.
Oh, that's a good point. If we want to solve this properly, we probably need to make the C++ `S3RetryStrategy` more inspectable. I can't really think of a simple and elegant design unfortunately.
Perhaps something like this:
```c++
class ARROW_EXPORT S3RetryStrategy {
public:
...
// "custom" by default, but could be "aws_standard" or "aws_default"
virtual const char* kind_name();
// Kind-specific serialization, requires some knowledge at the call site
// of each supported kind.
virtual std::string Serialize() = 0;
```
General-purpose serialization of C++ classes is not available, so Python would only support serializing the "aws_standard" and "aws_default" kinds.
@lidavidm Would you have a better approach?
--
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
[GitHub] [arrow] 3dbrows commented on pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
3dbrows commented on PR #13633:
URL: https://github.com/apache/arrow/pull/13633#issuecomment-1209035110
One way to sidestep this problem, of course, would be to _not_ specify a `retry_strategy` in `__init__` or `__reduce__` at all, and instead just hard-code the usage of AWS Standard retry strategy with, say, 5 max retries:
```
options.retry_strategy = CS3RetryStrategy.GetAwsStandardRetryStrategy(5)
```
In a way that feels like a cop-out but it's **still better** than the current approach, in that we are forcing a more sensible default than at present. The improvement is that we bake in a better retry mechanism.
Configurability could become a separate task for someone who (a) really wants it and (b) can come up with a neat PR to do 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] 3dbrows commented on a diff in pull request #13633: ARROW-17057: [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
3dbrows commented on code in PR #13633:
URL: https://github.com/apache/arrow/pull/13633#discussion_r941121194
##########
python/pyarrow/tests/test_fs.py:
##########
@@ -1107,6 +1107,10 @@ def test_s3_options():
assert isinstance(fs, S3FileSystem)
assert pickle.loads(pickle.dumps(fs)) == fs
+ fs = S3FileSystem(
+ retry_strategy=AwsStandardS3RetryStrategy(max_attempts=5))
+ assert isinstance(fs, S3FileSystem)
Review Comment:
Done.
--
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
[GitHub] [arrow] pitrou commented on pull request #13633: ARROW-17057: [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
pitrou commented on PR #13633:
URL: https://github.com/apache/arrow/pull/13633#issuecomment-1209202668
Probably? Also, I'm surprised: you don't run tests locally before pushing?
--
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
[GitHub] [arrow] pitrou commented on a diff in pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #13633:
URL: https://github.com/apache/arrow/pull/13633#discussion_r941116256
##########
python/pyarrow/tests/test_fs.py:
##########
@@ -1107,6 +1108,14 @@ def test_s3_options():
assert isinstance(fs, S3FileSystem)
assert pickle.loads(pickle.dumps(fs)) == fs
+ fs = S3FileSystem(
Review Comment:
```suggestion
# Note that the retry strategy won't survive pickling for now
fs = S3FileSystem(
```
--
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
[GitHub] [arrow] 3dbrows commented on a diff in pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
3dbrows commented on code in PR #13633:
URL: https://github.com/apache/arrow/pull/13633#discussion_r941115322
##########
python/pyarrow/tests/test_fs.py:
##########
@@ -1107,6 +1107,10 @@ def test_s3_options():
assert isinstance(fs, S3FileSystem)
assert pickle.loads(pickle.dumps(fs)) == fs
+ fs = S3FileSystem(
+ retry_strategy=AwsStandardS3RetryStrategy(max_attempts=5))
+ assert isinstance(fs, S3FileSystem)
Review Comment:
Done.
--
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
[GitHub] [arrow] 3dbrows commented on pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
3dbrows commented on PR #13633:
URL: https://github.com/apache/arrow/pull/13633#issuecomment-1209112131
@pitrou I have addressed the review comments - anything further to do?
--
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
[GitHub] [arrow] 3dbrows commented on pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
3dbrows commented on PR #13633:
URL: https://github.com/apache/arrow/pull/13633#issuecomment-1208059470
Added a test which is perhaps strange. It doesn't check whether we have assigned a particular kind of AWS strategy, as I'm not sure how we'd do that. Instead, it tests whether the strategy actually assigned matches the behaviour of the expected strategy. In other words, check it does retry after 1 attempt; does not after 6 attempts; and check the retry delay is in the expected range as per AWS docs.
This could be vastly simplified if someone knows how to mock out a static Cython method such that we can simply "assert that `CS3RetryStrategy.GetAwsStandardRetryStrategy()` was called with X as a parameter".
Alternatively, maybe it's _preferable_ that we're testing the behaviour here rather than what code is executed.
I'm not sure how to handle passing the strategy in the `__reduce__` method. A bit stuck there.
Very much open to suggestions on both points (or really: opinionated instructions on what to do). I think I have reached the limit of what I can do myself here without guidance.
--
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
[GitHub] [arrow] pitrou commented on pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
pitrou commented on PR #13633:
URL: https://github.com/apache/arrow/pull/13633#issuecomment-1208230858
User-defined strategies in C++ would simply not be serializable. However, user-defined strategies in Python probably would, since you can rely on `__reduce__`.
--
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
[GitHub] [arrow] 3dbrows commented on pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
3dbrows commented on PR #13633:
URL: https://github.com/apache/arrow/pull/13633#issuecomment-1187685147
Thanks @wjones127 . For safety's sake then, perhaps we want to introduce a new subclass of `S3RetryStrategy`, let's say `class AwsStockRetryStrategy : public S3RetryStrategy`, which has a constructor that takes an enum value e.g. `AwsStockRetryStategyEnum.Adaptive` or`AwsStockRetryStategyEnum.Standard`. Internally in this proposed `AwsStockRetryStrategy` class, we have a private member `std::shared_ptr<RetryStrategy>` (from AWS) and in the constructor we `new` up a subclass of that (`AdaptiveRetryStrategy`, `StandardRetryStrategy`) according to the passed-in enum value. Methods in this class simply call through to the private member instance's equivalents. I'll put together something on this... once the UK heat breaks, as I'm currently melting at my desk. 😅
Current unknowns:
* Finer points of surfacing the hypothetical `AwsStockRetryStrategy` in Python code
* How best to test
Thanks for looking at this.
--
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
[GitHub] [arrow] lidavidm commented on pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #13633:
URL: https://github.com/apache/arrow/pull/13633#issuecomment-1194415232
We could possibly add an `S3RetryStrategy::MakeFromAwsName` (or something like that) on the C++ side that will create an S3RetryStrategy which internally delegates to a built-in AWS RetryStrategy. That means there'll be a few layers of wrapping, but should avoid exposing anything we don't want to.
--
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
[GitHub] [arrow] 3dbrows commented on pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
3dbrows commented on PR #13633:
URL: https://github.com/apache/arrow/pull/13633#issuecomment-1205350024
@pitrou @lidavidm
Thank you for your patience and meticulous review; I very much appreciate the friendliness and constructiveness shown here, especially as a first-time contributor, and a C++/Cython noob at that.
Progress since last comments:
* Introduced `GetAwsDefaultRetryStrategy()` and `GetAwsStandardRetryStrategy()`
* These still have `static_cast<long>(max_attempts)` because the AWS SDK expects `long` - avoidable? (Would imagine `int64_t` and `long` have same precision?)
* These live in `S3RetryStrategy` as static methods.
* Possibly controversial: changing default behaviour of `s3fs` to use AWS Standard retry strategy
* This seems like a more sensible default and should not degrade current user experience.
* What prompted me to start this PR in the first place was the current default behaviour of retrying 3x in quick succession, which has suboptimal mitigation of `503 SlowDown`; I think using exponential backoff instead by default will improve reliability.
* Still to do:
* Testing.
* The JIRA ticket ARROW-17057 should be set to in-progress but I do not seem to have priveliges to do that.
Questions:
* Should I be defining `S3RetryStrategy::GetAwsDefaultRetryStrategy` or just `GetAwsDefaultRetryStrategy`? (`s3fs.cc`)
* Need to figure out how to encapsulate `retry_strategy` and `retry_max_attempts` nicely (avoid two parameters).
* How to handle `S3FileSystem._reconstruct()`: need to pass in the retry parameters?
--
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
[GitHub] [arrow] pitrou commented on pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
pitrou commented on PR #13633:
URL: https://github.com/apache/arrow/pull/13633#issuecomment-1209036441
@3dbrows Yes, that sounds reasonable.
--
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
[GitHub] [arrow] 3dbrows commented on pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
3dbrows commented on PR #13633:
URL: https://github.com/apache/arrow/pull/13633#issuecomment-1209049824
Sounds good @pitrou ; in that case I think we are finished, assuming the tests/code quality are acceptable.
--
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
[GitHub] [arrow] 3dbrows commented on a diff in pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
3dbrows commented on code in PR #13633:
URL: https://github.com/apache/arrow/pull/13633#discussion_r941116478
##########
python/pyarrow/tests/test_fs.py:
##########
@@ -1107,6 +1107,10 @@ def test_s3_options():
assert isinstance(fs, S3FileSystem)
assert pickle.loads(pickle.dumps(fs)) == fs
+ fs = S3FileSystem(
+ retry_strategy=AwsStandardS3RetryStrategy(max_attempts=5))
+ assert isinstance(fs, S3FileSystem)
Review Comment:
One sec, need to re-add the `ValueError` check.
--
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
[GitHub] [arrow] ursabot commented on pull request #13633: ARROW-17057: [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #13633:
URL: https://github.com/apache/arrow/pull/13633#issuecomment-1210902287
['Python', 'R'] benchmarks have high level of regressions.
[test-mac-arm](https://conbench.ursa.dev/compare/runs/9fd447b74d9f4d488ebf8f4aff896208...62345b2dfb5d41f890c2990b35cde93f/)
[ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/2ba641b5c26b4b0ead9ae2cb3a965217...10f7952f9f4541ff815d7452d2ba4e3a/)
--
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
[GitHub] [arrow] 3dbrows commented on pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
3dbrows commented on PR #13633:
URL: https://github.com/apache/arrow/pull/13633#issuecomment-1190061989
Going with a _much_ simpler approach where we can just give an enum value to select a stock strategy.
--
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
[GitHub] [arrow] wjones127 commented on a diff in pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
wjones127 commented on code in PR #13633:
URL: https://github.com/apache/arrow/pull/13633#discussion_r927972891
##########
python/pyarrow/_s3fs.pyx:
##########
@@ -283,6 +287,7 @@ cdef class S3FileSystem(FileSystem):
options.allow_bucket_creation = allow_bucket_creation
options.allow_bucket_deletion = allow_bucket_deletion
+ options.stock_retry_strategy = stock_retry_strategy
Review Comment:
Looking into this, it doesn't seem like we should need to modify the C++ code, since the current API will accept the AWS retry strategies. So what if we kept this logic at the Cython level. Something like:
```suggestion
if stock_retry_strategy == "standard":
options.retry_strategy = make_shared[AwsClientStandardRetryStrategy]()
elif stock_retry_strategy == "adaptive":
options.retry_strategy = make_shared[AwsClientAdaptiveRetryStrategy]()
elif isinstance(stock_retry_strategy, str):
raise ValueError(f"Unknown 'stock_retry_strategy': \"{stock_retry_strategy}\"")
```
You would also need to define `AwsClientStandardRetryStrategy` and others inside of `libarrow_fs.pxd`.
--
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
[GitHub] [arrow] lidavidm commented on pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #13633:
URL: https://github.com/apache/arrow/pull/13633#issuecomment-1194410411
The Arrow API shouldn't expose symbols from dependencies, so we can't subclass the AWS SDK's RetryStrategy from our own class.
--
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
[GitHub] [arrow] lidavidm commented on pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #13633:
URL: https://github.com/apache/arrow/pull/13633#issuecomment-1206780752
The static_cast is probably OK (maybe 32-bit windows needs an overflow check?)
--
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
[GitHub] [arrow] 3dbrows commented on pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
3dbrows commented on PR #13633:
URL: https://github.com/apache/arrow/pull/13633#issuecomment-1208159307
@github-actions autotune
--
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
[GitHub] [arrow] 3dbrows commented on a diff in pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
3dbrows commented on code in PR #13633:
URL: https://github.com/apache/arrow/pull/13633#discussion_r941063191
##########
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:
Yes, sorry, fighting a linter on my local machine. Will undo the import changes.
--
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
[GitHub] [arrow] 3dbrows commented on pull request #13633: ARROW-17057: [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
3dbrows commented on PR #13633:
URL: https://github.com/apache/arrow/pull/13633#issuecomment-1209211890
Unfortunately I am lost on how to set up a local development environment with all the right dependencies, and what test scripts to run etc., so I have relied on CI to do this 😅 (this is indeed slow!)
--
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
[GitHub] [arrow] ursabot commented on pull request #13633: ARROW-17057: [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #13633:
URL: https://github.com/apache/arrow/pull/13633#issuecomment-1210901976
Benchmark runs are scheduled for baseline = 74f221c925688a1fab05c0394818256d816ccfc1 and contender = ae071bbce5cb61c7553912f051d55ff5c7b45d7a. ae071bbce5cb61c7553912f051d55ff5c7b45d7a is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/0ac3170bf2eb4b8fae10567e39117e14...90f0c731774e4a6ea935995815012484/)
[Failed :arrow_down:1.91% :arrow_up:0.0%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/9fd447b74d9f4d488ebf8f4aff896208...62345b2dfb5d41f890c2990b35cde93f/)
[Failed :arrow_down:16.71% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/2ba641b5c26b4b0ead9ae2cb3a965217...10f7952f9f4541ff815d7452d2ba4e3a/)
[Finished :arrow_down:0.43% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/9fb9d7ee8e3842bea51b1f64303e0584...2fb0423d1b984b849bbf2ce52adf0ea5/)
Buildkite builds:
[Finished] [`ae071bbc` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/1289)
[Failed] [`ae071bbc` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/1304)
[Failed] [`ae071bbc` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/1287)
[Finished] [`ae071bbc` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/1303)
[Finished] [`74f221c9` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/1288)
[Failed] [`74f221c9` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/1303)
[Failed] [`74f221c9` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/1286)
[Finished] [`74f221c9` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/1302)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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
[GitHub] [arrow] 3dbrows commented on a diff in pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
3dbrows commented on code in PR #13633:
URL: https://github.com/apache/arrow/pull/13633#discussion_r928595053
##########
python/pyarrow/_s3fs.pyx:
##########
@@ -283,6 +287,7 @@ cdef class S3FileSystem(FileSystem):
options.allow_bucket_creation = allow_bucket_creation
options.allow_bucket_deletion = allow_bucket_deletion
+ options.stock_retry_strategy = stock_retry_strategy
Review Comment:
Thanks Will. This seems so obvious now you mention it. I'll come back with something to this effect.
--
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
[GitHub] [arrow] lidavidm commented on pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #13633:
URL: https://github.com/apache/arrow/pull/13633#issuecomment-1194416185
ConnectRetryStrategy is internal though (and already subclasses RetryStrategy), can we elaborate on what we're trying to do now?
--
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
[GitHub] [arrow] 3dbrows commented on pull request #13633: ARROW-17057 [Python] S3FileSystem has no parameter for retry strategy
Posted by GitBox <gi...@apache.org>.
3dbrows commented on PR #13633:
URL: https://github.com/apache/arrow/pull/13633#issuecomment-1194320102
No problem. I'm wondering if the best approach here is to ditch `S3RetryStrategy` altogether and re-write `ConnectRetryStrategy` (and others) to subclass `Aws::Client::RetryStrategy`. There might be context I am missing though as to why that is not the approach already taken. (Maybe testability is impaired in some fashion, thinking out loud.)
--
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