Skip to content

feat(io/s3): support connect timeout property#950

Open
mizoz wants to merge 1 commit intoapache:mainfrom
mizoz:fix/s3-connect-timeout
Open

feat(io/s3): support connect timeout property#950
mizoz wants to merge 1 commit intoapache:mainfrom
mizoz:fix/s3-connect-timeout

Conversation

@mizoz
Copy link
Copy Markdown

@mizoz mizoz commented Apr 28, 2026

Fixes #941.

Summary

  • Remove s3.connect-timeout from the unsupported S3 property path.
  • Parse s3.connect-timeout as a Go duration and apply it to the AWS SDK buildable client's dialer timeout.
  • Preserve existing proxy support by sharing the same buildable HTTP client when both proxy and connect timeout are configured.
  • Add focused tests for valid and invalid connect timeout values.

Testing

  • go test ./io/gocloud
  • git diff --check && go test ./...

Signed-off-by: A Z <zalabany3@gmail.com>
@mizoz mizoz requested a review from zeroshade as a code owner April 28, 2026 20:10
Copy link
Copy Markdown
Contributor

@laskoviymishka laskoviymishka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking this up:Dialer.Timeout is the right knob, and threading a single BuildableClient through the proxy and timeout branches is the right shape. Before merge I'd like to see one blocking item and two smaller items addressed:

Value format diverges from PyIceberg: s3.connect-timeout is a documented PyIceberg property whose value is seconds as a float (e.g. "60" or "60.0"; see pyiceberg/io/pyarrow.py and pyiceberg/io/fsspec.py, both of which call float(...), plus the 60.0 example in PyIceberg's configuration.md). time.ParseDuration rejects those values, so any property bag flowing from a Python writer to a Go reader — or any user copying values from PyIceberg docs — will hard-error here. Please accept the seconds-as-number form, with time.ParseDuration as a fallback for Go-flavored strings:

if s, err := strconv.ParseFloat(connectTimeout, 64); err == nil {
    timeout = time.Duration(s * float64(time.Second))
} else if d, derr := time.ParseDuration(connectTimeout); derr == nil {
    timeout = d
} else {
    return nil, fmt.Errorf("invalid s3 connect timeout %q (expected seconds, e.g. \"60\", or duration like \"5s\"): %w", connectTimeout, err)
}

It would be good to document the accepted forms next to the S3ConnectTimeout constant, and to add a test for "60" / "60.0".

Reject zero and negative durations. time.ParseDuration("0") succeeds and produces 0, which net.Dialer treats as "no timeout" — the opposite of what a user setting s3.connect-timeout=0 would expect. Negative values parse and make every dial fail with i/o timeout from far away. A if timeout <= 0 { return nil, fmt.Errorf(...) } check after parsing covers both.

Add a proxy + connect-timeout composition test. The structural reason to hoist httpClient into a shared variable is to compose proxy and timeout, but no test sets both. Please add a case that sets both S3ProxyURI and S3ConnectTimeout and asserts the dialer's Timeout and that the transport's Proxy is non-nil — this is the regression that would slip through silently in a future refactor.

Smaller things, non-blocking:

  • Removing the unsupportedS3Props guard is an observable behavior change for ParseAWSConfig (it used to return unsupported S3 property "..." for this key). Worth a release-notes line.
  • The proxy branch above uses '%s' and drops the url.Parse error; the new branch nicely uses %q + %w. Aligning the proxy branch in a follow-up would be a small but worthwhile cleanup — out of scope here.

Happy to re-review once the format/zero-value handling and composition test are in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: support s3.connect-timeout property

2 participants