westonpace commented on code in PR #13385:
URL: https://github.com/apache/arrow/pull/13385#discussion_r918425263
##########
cpp/src/arrow/filesystem/s3fs.cc:
##########
@@ -718,6 +718,14 @@ class ClientBuilder {
if (!options_.region.empty()) {
client_config_.region = ToAwsString(options_.region);
}
+ if (options_.request_timeout_ms != -1) {
+ client_config_.requestTimeoutMs =
std::lround(options_.request_timeout_ms);
+ }
+ if (options_.connect_timeout_ms != -1) {
+ client_config_.connectTimeoutMs =
std::lround(options_.connect_timeout_ms);
+ }
Review Comment:
```suggestion
if (options_.request_timeout_ms >= 0) {
client_config_.requestTimeoutMs =
std::lround(options_.request_timeout_ms);
}
if (options_.connect_timeout_ms >= 0) {
client_config_.connectTimeoutMs =
std::lround(options_.connect_timeout_ms);
}
```
We should probably either do `>=0` (suggestion above) or return an error if
it is less than `-1`
##########
cpp/src/arrow/filesystem/s3fs.h:
##########
@@ -102,6 +102,8 @@ struct ARROW_EXPORT S3Options {
/// the region (environment variables, configuration profile, EC2 metadata
/// server).
std::string region;
+ double request_timeout_ms = -1;
+ double connect_timeout_ms = -1;
Review Comment:
Can you add comments here? We should probably explain what the default is
(i.e. what happens if you leave this as -1) as well as what 0 means.
##########
python/pyarrow/_s3fs.pyx:
##########
@@ -179,7 +179,7 @@ cdef class S3FileSystem(FileSystem):
CS3FileSystem* s3fs
def __init__(self, *, access_key=None, secret_key=None, session_token=None,
- bint anonymous=False, region=None, scheme=None,
+ bint anonymous=False, region=None, request_timeout_ms=None,
connect_timeout_ms=None, scheme=None,
Review Comment:
Can you add these fields to the above documentation?
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
