tanmayrauth commented on code in PR #1141:
URL: https://github.com/apache/iceberg-go/pull/1141#discussion_r3328932909
##########
io/gocloud/s3_test.go:
##########
@@ -80,13 +84,111 @@ func TestParseAWSConfigRemoteSigningEnabled(t *testing.T) {
})
}
-func TestParseAWSConfigUnsupportedProperty(t *testing.T) {
+func TestParseAWSConfigInvalidConnectTimeout(t *testing.T) {
t.Parallel()
_, err := ParseAWSConfig(context.Background(), map[string]string{
- io.S3ConnectTimeout: "5000",
+ io.S3Region: "us-east-1",
+ io.S3ConnectTimeout: "not-a-duration",
})
- require.ErrorContains(t, err, "unsupported S3 property")
+ require.ErrorContains(t, err, "invalid s3.connect-timeout")
+}
+
+func TestParseAWSConfigConnectTimeout(t *testing.T) {
Review Comment:
Nice tests. One small gap: "60.0" proves a decimal point is tolerated but
doesn't actually exercise the fractional-seconds path — 60.0 *
float64(time.Second) collapses to the same int as 60. Consider adding a true
fractional case so a future change to the float math can't silently break
sub-second timeouts:
{
name: "fractional seconds",
timeout: "1.5",
want: 1500 * time.Millisecond,
},
##########
io/gocloud/s3.go:
##########
@@ -83,14 +73,32 @@ func ParseAWSConfig(ctx context.Context, props
map[string]string) (*aws.Config,
if proxy, ok := props[io.S3ProxyURI]; ok {
proxyURL, err := url.Parse(proxy)
if err != nil {
- return nil, fmt.Errorf("invalid s3 proxy url '%s'",
proxy)
+ return nil, fmt.Errorf("invalid s3 proxy url %q: %w",
proxy, err)
}
- opts = append(opts,
config.WithHTTPClient(awshttp.NewBuildableClient().WithTransportOptions(
+ httpClient = awshttp.NewBuildableClient().WithTransportOptions(
func(t *http.Transport) {
t.Proxy = http.ProxyURL(proxyURL)
},
- )))
+ )
+ }
+
+ if timeout, ok := props[io.S3ConnectTimeout]; ok {
+ duration, err := parseS3ConnectTimeout(timeout)
+ if err != nil {
+ return nil, err
+ }
+
+ if httpClient == nil {
+ httpClient = awshttp.NewBuildableClient()
Review Comment:
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?
##########
io/gocloud/s3.go:
##########
@@ -103,6 +111,27 @@ func ParseAWSConfig(ctx context.Context, props
map[string]string) (*aws.Config,
return awscfg, nil
}
+func parseS3ConnectTimeout(timeout string) (time.Duration, error) {
Review Comment:
Minor: the duration <= 0 check is duplicated in both branches. Easier to
read with a single validation at the end:
```
func parseS3ConnectTimeout(timeout string) (time.Duration, error) {
var duration time.Duration
if seconds, err := strconv.ParseFloat(timeout, 64); err == nil {
duration = time.Duration(seconds * float64(time.Second))
} else {
d, err := time.ParseDuration(timeout)
if err != nil {
return 0, fmt.Errorf("invalid s3.connect-timeout %q: must be
seconds as a number or a Go duration string", timeout)
}
duration = d
}
if duration <= 0 {
return 0, errors.New("s3.connect-timeout must be a positive
duration")
}
return duration, nil
}
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
