feat(lib-transfer-manager): add uploadDirectory functionality#8121
Conversation
ca50c40 to
a89984e
Compare
a89984e to
d13bff3
Compare
| * S3 key prefix prepended to each object key. | ||
| * By default, if no prefix is specified, objects are uploaded to the root of the bucket. | ||
| */ | ||
| s3Prefix?: string; |
There was a problem hiding this comment.
why does this one property get named with "s3" as a prefix?
everything in here is S3. This is more like keyPrefix.
There was a problem hiding this comment.
Named it according to the specification.
There was a problem hiding this comment.
refer to SEP as "spec" or specification in public
| * Return true to include the file for upload, false to skip. | ||
| * By default, if no filter is specified, all files will be uploaded. | ||
| */ | ||
| filter?: (filePath: string) => boolean; |
| * Max concurrent file uploads for this directory operation. | ||
| * Default: 100. | ||
| */ | ||
| maxConcurrency?: number; |
There was a problem hiding this comment.
why is this 100? the default MPU doesn't even have 100 concurrency
There was a problem hiding this comment.
That was the default specified in the SEP. For file level concurreny, not part-level.But for directories with many large files the memory could be high.
There was a problem hiding this comment.
for small files this will call PutObject with file stream input rather than MPU, right?
The memory usage in that case is mostly stream chunk buffers? I think if MPU is triggered by a large file in the directory, it should count more than 1 against the concurrency limit.
15ee262 to
9afa92d
Compare
| * @internal | ||
| */ | ||
| export class Semaphore { | ||
| private queue: Array<{ weight: number; resolve: () => void }> = []; |
There was a problem hiding this comment.
Problem I can think of here with FIFO based ordering is if a file needs more slots than available, it will also block the requests behind it which might need a fewer slots. But if we let smaller files go first, it might lead to large files waiting indefinitely.
There was a problem hiding this comment.
Updated the semaphore to allow a request to proceed as long as inFlightRequest count is less than concurrency limit.
bd2115b to
d0e8007
Compare
| * Uses fs.opendir with recursive option (Node 20+). | ||
| * Handles symlink cycle detection when followSymbolicLinks is true. | ||
| * | ||
| * @internal |
There was a problem hiding this comment.
minor issue - private methods don't need the @internal tag.
| transferOptions?: TransferOptions; | ||
| }): Promise<{ objectsUploaded: number; objectsFailed: number }> { | ||
| throw new Error("Method not implemented."); | ||
| public async uploadDirectory(request: UploadDirectoryRequest, transferOptions?: TransferOptions): Promise<UploadDirectoryResponse> { |
There was a problem hiding this comment.
this being a method makes TM inherently unusable in browsers, right?
perhaps this method should dispatch to a injectable implementation which throws 'not implemented' in browsers?
There was a problem hiding this comment.
Yes, currently upload (threadUploadInParts, threadedUploadInPartsFromFile paths) and uploadAll doesn't have browser support, considering the less no of customers we have that use the SDK in browsers. But will add it later.
Issue
Internal JS-6110
Description
Adds
uploadDirectoryfunctionality to S3TransferManager for uploading files from a local directory to an S3 bucket.Testing
Checklist
*.integ.spec.ts) or E2E tests.@public,@internaltags and enabled doc generation on the package. Remember that access level annotations go below the description, not above.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.