Skip to content

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

Open
Revanth14 wants to merge 1 commit into
apache:mainfrom
Revanth14:feat/s3-connect-timeout
Open

feat(io/s3): support connect timeout property#1141
Revanth14 wants to merge 1 commit into
apache:mainfrom
Revanth14:feat/s3-connect-timeout

Conversation

@Revanth14
Copy link
Copy Markdown

Fixes #941

Summary

  • Support the s3.connect-timeout property for S3 clients.
  • Accept PyIceberg-compatible numeric seconds such as 60 and 60.0, plus Go duration strings such as 5s.
  • Apply the value to the AWS SDK buildable client's dialer timeout.
  • Compose connect timeout with s3.proxy-uri by sharing the same buildable HTTP client.
  • Reject zero and negative timeout values with a clear error.

Supersedes #950 and addresses all three items from laskoviymishka's review: float-seconds parsing, zero/negative rejection, and proxy+timeout composition test.

Testing

  • go test ./io/gocloud/...

Release note

S3 now supports the s3.connect-timeout property, accepting numeric seconds like 60/60.0 and Go duration strings like 5s.

@Revanth14 Revanth14 requested a review from zeroshade as a code owner May 30, 2026 07:44
@Revanth14 Revanth14 force-pushed the feat/s3-connect-timeout branch from 5fdb93c to f145e50 Compare May 30, 2026 07:50
@Revanth14
Copy link
Copy Markdown
Author

@laskoviymishka @tanmayrauth
This should address the feedback from #950. Could you please review when you have some time?

Comment thread io/gocloud/s3_test.go
Comment thread io/gocloud/s3.go
Comment thread io/gocloud/s3.go
}

if httpClient == nil {
httpClient = awshttp.NewBuildableClient()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not a blocker for this PR since the same pattern already exists for s3.proxy-uri, but worth flagging: createS3Bucket (lines 167-176) applies tuned transport options (MaxIdleConnsPerHost: 256, MaxIdleConns: 256, etc.) only when awscfg.HTTPClient == nil. Setting s3.connect-timeout now populates HTTPClient here, so callers fall back to the AWS SDK's awshttp.NewBuildableClient() defaults — notably MaxIdleConnsPerHost: 10 instead of 256, which can hurt highly parallel scans. Could you open a follow-up issue to apply the same transport tuning inside ParseAWSConfig (or factor a shared newBuildableClient() helper) so the proxy and connect-timeout paths don't silently regress connection pooling?

@Revanth14 Revanth14 force-pushed the feat/s3-connect-timeout branch from f145e50 to a28aa17 Compare May 30, 2026 20:47
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