Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat S3 Transfer Manager v2 GetObject/DownloadObject #2996

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

wty-Bryant
Copy link
Contributor

Implement v2 s3 transfer manager's GetObject/DownloadObject api bound to single union client which mimics normal service client's initialization and api call. User could now download object sequentially with GetObject or asynchronously with DownloadObject.

Test: passed unit test for GetObject and DownloadObject, passed integration test of uploading object via s3 client and validating its content after downloading through v2 transfer manager.

wty-Bryant and others added 25 commits January 31, 2025 16:16
* recommit transfer manager v2 files

* change pool to store slice pointer

* add integ test for putobject

* update go mod

* minor changes for v0.1.0

* update tags

* update tags

* update integ test dependency version

* change err var name

* update go mod

* change input/output type comment

* minor change

---------

Co-authored-by: Tianyi Wang <[email protected]>

rebase from main

rebase branch from main
move transfer manager v2 integration tests to within module

move putobject integ test to transfer manager module
add getobject integ test
@wty-Bryant wty-Bryant requested a review from a team as a code owner January 31, 2025 21:58
@@ -20,6 +20,8 @@ const defaultMultipartUploadThreshold = 1024 * 1024 * 16
// using PutObject().
const defaultTransferConcurrency = 5

const defaultPartBodyMaxRetries = 3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this copied over from existing implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, the latest SEP doesn't have such retry limitation in detail

}

// GetObjectInput represents a request to the GetObject() or DownloadObject() call. It contains common fields
// of s3 GetObject input
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems incomplete, of this and what else?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing else, GetObjectInput is the only s3 client input used here

feature/s3/transfermanager/api_op_GetObject.go Outdated Show resolved Hide resolved
feature/s3/transfermanager/api_op_GetObject.go Outdated Show resolved Hide resolved
for _, opt := range opts {
opt(&i.options)
}
i.options.Concurrency = 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we setting Concurrency to 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetObject here mimics s3.GetObject to get an object's chunks and write to output body in sequence, thus there should be only one goroutine to write chunks to output in order

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants