Feat S3 Transfer Manager v2 UploadDirectory #3110
Conversation
wty-Bryant
force-pushed
the
feat-keyprefix-upload
branch
from
775db9a to
7f51d9c
Compare
| postprocessFunc := func() error { | ||
| // this cleans up all possible symlinks regardless of | ||
| // whether or not it is successfully created | ||
| os.Remove(symlinkPath1) |
There was a problem hiding this comment.
curious why do we need to create and delete symlinks in tests. I suspect there are some gotchas when using git across multiple platforms, but want to confirm
There was a problem hiding this comment.
that's why I create and delete them dynamically in test code, different platforms have different file separator and location to put go v2, so a static symlink will be invalid in that case.
|
|
||
| // The s3 delimeter contatenating each object key based on local file separator | ||
| // and file's relative path | ||
| S3Delimiter string |
There was a problem hiding this comment.
maybe worth mentioning that this delimiter cannot be included in another directory. It makes sense, but took me by surprise reading the tests
| } else { | ||
| key = keyPrefix + u.in.S3Delimiter + filepath.Base(path) | ||
| } | ||
| fileInfo, err := os.Lstat(absPath) |
There was a problem hiding this comment.
shouldn't we check this err?
| if err != nil { | ||
| return []string{}, err | ||
| } | ||
| subFiles, err := f.Readdir(0) |
There was a problem hiding this comment.
the docs on Readdir say
Most clients are better served by the more efficient ReadDir method.
However, there may be a reason why we're using this instead
There was a problem hiding this comment.
we only need sub file name, change to more efficient one
|
|
||
| func (u *directoryUploader) uploadDirectory(ctx context.Context) (*UploadDirectoryOutput, error) { | ||
| u.init() | ||
| ch := make(chan fileChunk) |
There was a problem hiding this comment.
I don't think we're doing file chunking, so I would call this just fileChan or something similar.
| break | ||
| } | ||
| if u.getErr() != nil { | ||
| continue |
There was a problem hiding this comment.
Curious about the continue here. I think the idea is to continue uploading files if there's an error with the current file, but I don't see a place where we clear an error once it has been set, so wouldn't this always return non-nil once it has been set?
There was a problem hiding this comment.
changed with break since an undrained channel will still be gc once closed and not referred by any struct
| Delimiter string | ||
| KeyPrefix string | ||
| ExpectFilesUploaded int | ||
| ExpectFilesFailed int |
There was a problem hiding this comment.
we don't have any test that uses this
wty-Bryant
force-pushed
the
feat-keyprefix-upload
branch
from
7f51d9c to
8d95b62
Compare
Implement v2 s3 transfer manager's UploadDirectory api bound to single union client which mimics normal service client's initialization and api call. User could now upload a directory's files recursively/non-recursively to S3 with customized filtering and key naming in a single operation call.
Test: This PR passed unit tests for UploadDirectory which covers use cases with different config and folder structure to confirm this key-prefix uploader could find valid file location, generate correct key and pass upload requests to S3 concurrently. It also passed integration test which uploads source directory containing different sizes files to S3 via new directory uploader, then downloads objects back using S3 client and checks their content consistency