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.

Sign up for GitHub

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

Jump to bottom

always use multipart uploader for s3 uploads #13866

Merged
merged 1 commit into from
Jan 29, 2019
Merged

always use multipart uploader for s3 uploads #13866

merged 1 commit into from
Jan 29, 2019

Conversation

icewind1991
Copy link
Member

@icewind1991 icewind1991 commented Jan 28, 2019 •
edited by MorrisJobke
Loading

the multipart uploader handles non seekable streams while upload does not

Fixes #13062

@icewind1991
always use multipart uploader for s3 uploads
1d322d3 
the multipart uploader handles non seekable streams while `upload` does not

Signed-off-by: Robin Appelman <[email protected]>
@icewind1991 icewind1991 added the 3. to review Waiting for reviews label Jan 28, 2019
@icewind1991 icewind1991 added this to the Nextcloud 16 milestone Jan 28, 2019
@icewind1991
Copy link
Member Author

did some quick "real life" testing, chunked uploads work as expected with this patch while they break on master

@icewind1991
Copy link
Member Author

it might still be useful to look into making the assembly stream seekable, as I expect that that would lead to faster uploading than having the multipart uploader work around it

MorrisJobke
MorrisJobke approved these changes Jan 28, 2019
View reviewed changes
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Code makes sense 👍

@MorrisJobke MorrisJobke mentioned this pull request Jan 28, 2019
Closed
@MorrisJobke
Copy link
Member

MorrisJobke commented Jan 28, 2019 •
edited
Loading

@icewind1991 Does this fix #13062?

@icewind1991
Copy link
Member Author

yes

rullzer
rullzer approved these changes Jan 29, 2019
View reviewed changes
Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

Tested and works! 🚀

@rullzer rullzer merged commit 0fd208a into master Jan 29, 2019
@rullzer rullzer deleted the s3-multipart-uploader branch January 29, 2019 10:14
@rullzer
Copy link
Member

rullzer commented Jan 29, 2019

/backport to stable15

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Jan 29, 2019
Merged
@backportbot-nextcloud
Copy link

backport to stable15 in #13882

@HLFH HLFH mentioned this pull request Feb 8, 2019
Closed
@HLFH
Copy link
Contributor

HLFH commented Feb 11, 2019

@MorrisJobke @icewind1991 Hi. Does this PR fix #8275?

@MorrisJobke
Copy link
Member

@MorrisJobke @icewind1991 Hi. Does this PR fix #8275?

Does not look like, because this PR fixes uploads while the linked issue is most likely about downloads of images.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MorrisJobke MorrisJobke MorrisJobke approved these changes

@rullzer rullzer rullzer approved these changes

@blizzz blizzz Awaiting requested review from blizzz

Assignees
No one assigned
Labels
3. to review Waiting for reviews feature: filesystem
Projects
None yet
Milestone
Nextcloud 16
Development

Successfully merging this pull request may close these issues.
4 participants
@icewind1991 @MorrisJobke @rullzer @HLFH