-
Notifications
You must be signed in to change notification settings - Fork 565
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(lib-storage): rewrite lib-storage upload #2039
Conversation
Hi @alexforsyth, will you incorporate the performance improvements from my PR at #1955 ? |
@monken yes! I'll request you as a reviewer when this PR is in better shape. Everything should be using for...await syntax here. I'm looking at the buffer concat in readable chunker right now. The way you do it is much better, I'll only concat once. |
Codecov Report
@@ Coverage Diff @@
## master #2039 +/- ##
==========================================
+ Coverage 79.30% 79.57% +0.26%
==========================================
Files 368 365 -3
Lines 15132 15130 -2
Branches 3222 3251 +29
==========================================
+ Hits 12001 12039 +38
+ Misses 3131 3091 -40
Continue to review full report at Codecov.
|
f270f5b
to
ac5c199
Compare
@monken it looks like I cannot add you as a reviewer to this PR. I'd still love your feedback at any rate |
Thank you so much for your attention here, since I only was able to give partial care previously and we've been waiting for these performance improvements to switch over to the new SDK. If you care, I'll get this tested with my use case some point this week. |
@samsullivan that'd be great! We're looking to get this out with the next release |
Co-authored-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com>
Please check that it supports server-side encryption with |
@markb-trustifi it should, all the params you normally pass to multipart upload should work just the same as they are simply passed through to the underlying client. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't have time to review the code changes, but I 💯 agree that this needed to be rewritten and I appreciate your work. I built you're branch in my test build and threw this into our stage AWS environment just now and it worked fine!
Super excited to have this released. v2 of the JS SDK has an ECS Fargate bug where it doesn't use the IAM role on the container, so I'll get to remove some access keys once upgraded 🙂
@monken @samsullivan @xfournet Thanks so much for the engagement. It's awesome to see the involvement. We're aiming at releasing these changes soon |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
FYI: This should be out with |
Confirmed working in 3.6.0, thanks! |
☝️ same here, really appreciate the effort by everybody on this one. Happy to be off of v2 SDK! |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread. |
Issue
Description
As part of a review of our codebase I rewrote Upload(). This PR fixes a range of issues and should address most of the Issues filed (below). This rewrite is backwards compatible.
Upload is a wrapper on s3.multipart upload. This is largely analogous to managed upload in V2 of the JS SDK.
Here's how you use Upload:
Testing
How was this change tested?
Upload
. #1891Manual Browser testing
Manual Node testing
Additional context
Add any other context about the PR here.
PRs:
Fixes: #1990
Fixes: #1955
Issues:
Fixes: #1973
Fixes: #1891
Fixes: #1729
Fixes: #1688
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.