s3: add support for multipart uploads

s3: add support for multipart uploads



https://code.launchpad.net/~niemeyer/goamz/multipart-uploads/+merge/146043

Requires: https://code.launchpad.net/~niemeyer/goamz/introduce-retrying/+merge/146042

(do not edit description out of merge proposal)

[niemeyer, 2013-02-01 06:00:20]

Please take a look.

[jameinel, 2013-02-04 09:37:41]

Overall LGTM.

https://codereview.appspot.com/7237068/diff/1/s3/multi.go
File s3/multi.go (right):

https://codereview.appspot.com/7237068/diff/1/s3/multi.go#newcode122
s3/multi.go:122: // Each part, except for the last one, must be at least 5MB in
size.
It took me a while to find this limitation, but I did find it
http://docs.aws.amazon.com/AmazonS3/latest/dev/qfacts.html

maybe we could link to that as well.

https://codereview.appspot.com/7237068/diff/1/s3/multi.go#newcode124
s3/multi.go:124: // See http://goo.gl/pqZer for details.
Is there a reason to use shortened URLs? It seems like you have plenty of space
in source code, and it lets you know something about the link before following
it. (what website, what doc, etc)

https://codereview.appspot.com/7237068/diff/1/s3/multi.go#newcode125
s3/multi.go:125: func (m *Multi) PutPart(n int, r io.ReadSeeker) (Part, error) {
I wonder if it would be nice to have one big file and then you pass in "start at
X, part-upload 10MB", rather than requiring the user to split everything locally
first.

I think you could do that if you passed in the start+length along with the
io.ReadSeeker. Or just had the ReadSeeker call Tell instead of Seek at the
beginning, and then take some sort of "max length".

Not that you have to do it, but it seems helpful if you had a big file to
upload, to not have to split it yourself.

https://codereview.appspot.com/7237068/diff/1/s3/multi.go#newcode162
s3/multi.go:162: return Part{n, resp.Header.Get("ETag"), length}, nil
The ETag seems particularly important in the response (as it is the handle you
can use later to do something with it). But it doesn't seem like you validate
that the ETag is not the empty string.
Would that be worth checking here?

[rog, 2013-02-04 11:25:48]

LGTM

https://codereview.appspot.com/7237068/diff/1/s3/multi.go
File s3/multi.go (right):

https://codereview.appspot.com/7237068/diff/1/s3/multi.go#newcode19
s3/multi.go:19: // See http://goo.gl/vJfTG for an overview on multipart uploads.
"on" isn't quite right here.

how about just: "See http://goo.gl/vJfTG for an overview."?

https://codereview.appspot.com/7237068/diff/1/s3/multi.go#newcode125
s3/multi.go:125: func (m *Multi) PutPart(n int, r io.ReadSeeker) (Part, error) {
On 2013/02/04 09:37:42, jameinel wrote:
> I wonder if it would be nice to have one big file and then you pass in "start
at
> X, part-upload 10MB", rather than requiring the user to split everything
locally
> first.

i'm not sure. i think that io.SectionReader would make that quite simple.

[niemeyer, 2013-02-05 12:50:00]

https://codereview.appspot.com/7237068/diff/1/s3/multi.go
File s3/multi.go (right):

https://codereview.appspot.com/7237068/diff/1/s3/multi.go#newcode19
s3/multi.go:19: // See http://goo.gl/vJfTG for an overview on multipart uploads.
On 2013/02/04 11:25:48, rog wrote:
> "on" isn't quite right here.
> 
> how about just: "See http://goo.gl/vJfTG for an overview."?

Will do.

https://codereview.appspot.com/7237068/diff/1/s3/multi.go#newcode122
s3/multi.go:122: // Each part, except for the last one, must be at least 5MB in
size.
On 2013/02/04 09:37:42, jameinel wrote:
> It took me a while to find this limitation, but I did find it
> http://docs.aws.amazon.com/AmazonS3/latest/dev/qfacts.html
> 
> maybe we could link to that as well.

This is part of the documentation of UploadPart too (linked below).

https://codereview.appspot.com/7237068/diff/1/s3/multi.go#newcode125
s3/multi.go:125: func (m *Multi) PutPart(n int, r io.ReadSeeker) (Part, error) {
On 2013/02/04 09:37:42, jameinel wrote:
> I wonder if it would be nice to have one big file and then you pass in "start
at
> X, part-upload 10MB", rather than requiring the user to split everything
locally
> first.
> 
> I think you could do that if you passed in the start+length along with the
> io.ReadSeeker. Or just had the ReadSeeker call Tell instead of Seek at the
> beginning, and then take some sort of "max length".
> 
> Not that you have to do it, but it seems helpful if you had a big file to
> upload, to not have to split it yourself.

Agreed. I'm thinking about having a simplified interface based on this one next,
so that you can actually just pass a ReadSeeker in, and it will do the whole
thing: figure out which parts have been sent, build the parts that haven't been
sent, and send them. On the client side this should be as simple as
PutAll(seeker) or something.

https://codereview.appspot.com/7237068/diff/1/s3/multi.go#newcode162
s3/multi.go:162: return Part{n, resp.Header.Get("ETag"), length}, nil
On 2013/02/04 09:37:42, jameinel wrote:
> The ETag seems particularly important in the response (as it is the handle you
> can use later to do something with it). But it doesn't seem like you validate
> that the ETag is not the empty string.
> Would that be worth checking here?

Sounds good, will do.

[niemeyer, 2013-02-11 17:16:56]

Please take a look.

[rog, 2013-02-11 17:28:04]

On 2013/02/11 17:16:56, niemeyer wrote:
> Please take a look.

LGTM still

[niemeyer, 2013-02-11 17:39:08]

*** Submitted:

s3: add support for multipart uploads

R=jameinel, rog
CC=
https://codereview.appspot.com/7237068