increase timeout to 30s #3422
increase timeout to 30s #3422
Conversation
|
Are we sure that the previous timeout is wrong? My concern is that these timeouts were caused by something other than a bunch of large objects, so if we do this then we won't scan any more data but each scan will take six times as long. |
According to the documentation for the library we're using: I believe the timeout was initially set under the assumption that we were only retrieving metadata, but the docs confirm it retrieves the entire object. I'm not certain what else might be causing the context deadline. |
|
Adjusted the logger to use our ctx. I also included the object size in the logger kvp. |
There was a problem hiding this comment.
This seems good, just had a few non-blocking questions.
One other thought: can log be removed from the struct now, or is it still referenced in other parts? Not saying it should be removed in this PR, but I saw you corrected many of the log calls to use the passed ctx.
| @@ -131,6 +131,7 @@ func (s *Source) setMaxObjectSize(maxObjectSize int64) { | |||
| func (s *Source) newClient(region, roleArn string) (*s3.S3, error) { | |||
| cfg := aws.NewConfig() | |||
| cfg.CredentialsChainVerboseErrors = aws.Bool(true) | |||
| cfg.LogLevel = aws.LogLevel(aws.LogDebugWithRequestErrors) | |||
There was a problem hiding this comment.
Was this just for testing or are we keeping it in?
There was a problem hiding this comment.
I thought it would be useful even for non-debugging purposes since it would only log during errors. What do you think?
There was a problem hiding this comment.
Yeah, makes sense to me.
pkg/sources/s3/s3.go
Outdated
| } | ||
| obj := *o |
There was a problem hiding this comment.
Why do we need to make a shallow copy here?
There was a problem hiding this comment.
Oh whoops, I forgot to remove this. I was testing something. 😅 Thanks.
Unfortunately, it's used across the rest of the source 😢 I can clean it up in a separate PR. |
Description:
We're seeing frequent context timeout errors during
GetObjectAPI calls. This PR increases the default timeout from 5 to 30 seconds.Error:
Checklist:
make test-community)?make lintthis requires golangci-lint)?