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 PutObject #2733

Open
wants to merge 2 commits into
base: feat-transfer-manager-v2
Choose a base branch
from

Conversation

wty-Bryant
Copy link
Contributor

Implement v2 s3 transfer manager's putobject api bound to single union client which mimics normal service client's initialization and api call

@wty-Bryant wty-Bryant requested a review from a team as a code owner August 4, 2024 21:05
@lucix-aws
Copy link
Contributor

lucix-aws commented Aug 5, 2024

Tests are failing.

I know that some of the business logic is duplicated from the original version here, but I'm reviewing it as if it's all new.

.changelog/f48bec23878347028d9d4b5ef03e0757.json Outdated Show resolved Hide resolved
feature/s3/transfermanager/api.go Outdated Show resolved Hide resolved
feature/s3/transfermanager/internal/testing/endpoints.go Outdated Show resolved Hide resolved
feature/s3/transfermanager/pool.go Show resolved Hide resolved
feature/s3/transfermanager/putobject.go Outdated Show resolved Hide resolved
feature/s3/transfermanager/pool.go Outdated Show resolved Hide resolved
feature/s3/transfermanager/putobject.go Outdated Show resolved Hide resolved
feature/s3/transfermanager/putobject.go Outdated Show resolved Hide resolved
feature/s3/transfermanager/putobject.go Outdated Show resolved Hide resolved
feature/s3/transfermanager/pool.go Show resolved Hide resolved
feature/s3/transfermanager/transfermanager.go Outdated Show resolved Hide resolved
feature/s3/transfermanager/putobject.go Outdated Show resolved Hide resolved
feature/s3/transfermanager/putobject.go Outdated Show resolved Hide resolved
feature/s3/transfermanager/api_client.go Outdated Show resolved Hide resolved
feature/s3/transfermanager/api_client.go Outdated Show resolved Hide resolved
feature/s3/transfermanager/api_client.go Outdated Show resolved Hide resolved
feature/s3/transfermanager/api_op_PutObject.go Outdated Show resolved Hide resolved
feature/s3/transfermanager/api_op_PutObject.go Outdated Show resolved Hide resolved
feature/s3/transfermanager/api_op_PutObject.go Outdated Show resolved Hide resolved
feature/s3/transfermanager/api_op_PutObject.go Outdated Show resolved Hide resolved
feature/s3/transfermanager/api_op_PutObject.go Outdated Show resolved Hide resolved
Copy link
Contributor

@lucix-aws lucix-aws left a comment

Choose a reason for hiding this comment

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

We need to seriously re-examine the part pool behavior. It seems entirely overcomplicated for what we need out of it.

We need to discuss how we manage maximum part size of 10k for MPUs - failing on an in-progress download from exceeding the limit after the fact isn't acceptable behavior.

feature/s3/transfermanager/api.go Outdated Show resolved Hide resolved
feature/s3/transfermanager/api.go Outdated Show resolved Hide resolved
feature/s3/transfermanager/api_op_PutObject.go Outdated Show resolved Hide resolved
feature/s3/transfermanager/api_op_PutObject.go Outdated Show resolved Hide resolved
feature/s3/transfermanager/api_op_PutObject.go Outdated Show resolved Hide resolved
feature/s3/transfermanager/pool.go Outdated Show resolved Hide resolved
feature/s3/transfermanager/types.go Outdated Show resolved Hide resolved
feature/s3/transfermanager/types.go Outdated Show resolved Hide resolved
feature/s3/transfermanager/api_op_PutObject.go Outdated Show resolved Hide resolved
feature/s3/transfermanager/api_op_PutObject.go Outdated Show resolved Hide resolved
@wty-Bryant wty-Bryant force-pushed the feat-tmv2-putobject branch 2 times, most recently from f3a43fb to ba7b247 Compare October 31, 2024 02:14
feature/s3/transfermanager/pool.go Show resolved Hide resolved
feature/s3/transfermanager/pool.go Show resolved Hide resolved
@@ -0,0 +1,48 @@
package transfermanager
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we get integration tests?

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