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

AwsS3 Multipart Uploads #513

Open
wants to merge 1 commit into
base: master
from
Open

Conversation

@MonsieurLanza
Copy link

MonsieurLanza commented Jun 30, 2017

This is a modification of AwsS3 adapter to allow multipart uploads when files are bigger than a certain size.

Size limits / part size are passed as options to the adapter and set to defaults, respectively 5Go and
5Mo. 5Go being the maximum size AWS S3 accepts in a single upload, and 5Mo the minimum part size.

Limitations : won't work with string content, you have to pass a resource handle otherwise write() will return false. We may use InMemoryStream to bypass this limitation ?

PHP7.1 fails on docker image, not sure why. May need some help on this.

Copy link
Contributor

nicolasmure left a comment

I just have some small comments, but thank you @MonsieurLanza 👍 .
Still, I'm waiting for other reviews ;)

* Amazon S3 does not not allow uploads > 5Go
*/
const MAX_CONTENT_SIZE = 5368709120;
const DEFAULT_PART_SIZE = 5 * 1024 * 1024;

This comment has been minimized.

Copy link
@nicolasmure

nicolasmure Jul 1, 2017

Contributor

Can you set this const to the computed value directly please ? (avoids to calculate it ; add a comment if needed to know where this value comes from)

{
$options = $this->getOptions($key);
$result = $this->service->createMultipartUpload($options);
return $result['UploadId'];

This comment has been minimized.

Copy link
@nicolasmure

nicolasmure Jul 1, 2017

Contributor

Please add an empty line before this return statement 😉

} else {
/*
* content is a string & too big to be uploaded in one shot
* We may want to throw an exception here ?

This comment has been minimized.

Copy link
@nicolasmure

nicolasmure Jul 1, 2017

Contributor

It would lead into a BC break IMO.

);

$options = $this->getOptions($key, $options);
return $this->service->uploadPart($options);

This comment has been minimized.

Copy link
@nicolasmure

nicolasmure Jul 1, 2017

Contributor

Same here : please insert an empty line before the return statement 😉

{
$uploadId = $this->initiateMultipartUpload($key);

$parts = array();

This comment has been minimized.

Copy link
@nicolasmure

nicolasmure Jul 1, 2017

Contributor

Please use short array [] syntax 😉

@MonsieurLanza

This comment has been minimized.

Copy link
Author

MonsieurLanza commented Jul 2, 2017

Hope I did not forget anything :)

@nicolasmure

This comment has been minimized.

Copy link
Contributor

nicolasmure commented Jul 3, 2017

Can you rebase on master (to consider the merge of #514 ), and update your test please (note the changes)?

@MonsieurLanza MonsieurLanza force-pushed the meero-com:aws-multipart branch from c4a51f7 to 7bef756 Jul 3, 2017
@MonsieurLanza

This comment has been minimized.

Copy link
Author

MonsieurLanza commented Jul 4, 2017

Done.

@MonsieurLanza

This comment has been minimized.

Copy link
Author

MonsieurLanza commented Jul 4, 2017

Fix : Use Amazon's size limit for string upload and option specific limit for resource handle to allow 5Go upload no matter what.
Added a test case to cover that.

@MonsieurLanza

This comment has been minimized.

Copy link
Author

MonsieurLanza commented Jul 19, 2017

Hello, some news here ?
We would like this PR to get rid of our custom repo. :)

@nicolasmure nicolasmure mentioned this pull request Sep 21, 2017
0 of 13 tasks complete
@amcastror

This comment has been minimized.

Copy link

amcastror commented Mar 1, 2018

Hi, are there plans to merge this? I'm having troubles with this.

Thanks!

@JoydS

This comment has been minimized.

Copy link

JoydS commented Mar 28, 2018

Hi guys,
Is this possible to merge this pull request ? Need it!!
Thank you

@jfouca jfouca force-pushed the meero-com:aws-multipart branch from 2686dc8 to b8738c3 Nov 6, 2019
@jfouca jfouca force-pushed the meero-com:aws-multipart branch from b8738c3 to 080a2cf Nov 6, 2019
@jfouca

This comment has been minimized.

Copy link

jfouca commented Nov 6, 2019

@NiR- we would really appreciate your review on this since @nicolasmure approved it some time ago :)

thank you

@amodliszewski

This comment has been minimized.

Copy link

amodliszewski commented Jan 17, 2020

Hello, any idea when this would be merged? Love the concept, would release some workarounds form my project.

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

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.