source-s3: use context for file listing #350
Conversation
a346d6a to
e57eb48
Compare
source-s3/main.go
Outdated
| }, nil | ||
| } | ||
|
|
||
| return filesource.ListingFunc(func() (filesource.ObjectInfo, error) { |
There was a problem hiding this comment.
It feels a little bit weird to construct an s3Listing iterator and then wrap it in a closure. Perhaps add a ctx field to s3Listing instead?
There was a problem hiding this comment.
Yeah I was trying perhaps a little too hard to not add a context field to a struct, but in this case it is probably more clear, and not that big of a deal anyway. Updated!
There was a problem hiding this comment.
Yeah, I agree with that guidance in general but IMO storing the context in a closure is morally equivalent to storing it in the struct proper.
The way to actually keep the spirit of those guidelines would be to refactor the listing iterator interface so it's .Next(ctx), but I didn't recommend that because it looks like other filesource implementations wouldn't be able to use that (for example, see source-gcs which takes the context from List() and makes that part of its *storage.ObjectIterator)
source-s3/main.go
Outdated
| @@ -188,7 +192,7 @@ func (l *s3Listing) poll() error { | |||
| input.ContinuationToken = l.output.NextContinuationToken | |||
| } | |||
|
|
|||
| if out, err := l.s3.ListObjectsV2(&input); err != nil { | |||
| if out, err := l.s3.ListObjectsV2WithContext(context.Background(), &input); err != nil { | |||
There was a problem hiding this comment.
It feels like this was intended to use ctx rather than context.Background()?
Closes #345
Description:
Threads the context through to the
source-s3connectorListoperation. #344 accomplished the same thing forRead. This will allow for better handling of context cancellations or timeouts.Workflow steps:
Should be minimal observable impact. This will eliminate the potential for very large and/or slow file listings to lock up schema discovery indefinitely.
Documentation links affected:
N/A
Notes for reviewers:
I had initially thought this would take more work to get done, but it turned out to be pretty simple with the
filesource.ListingFuncadapter already available. There wasn't much possible improvement I could see from refactoring the existing iterator code.This change is