s3: Multi.PutAll for auto-chunking and resuming

s3: Multi.PutAll for auto-chunking and resuming

Bucket.Multi method also added for trivial resume-or-init handling.

https://code.launchpad.net/~niemeyer/goamz/put-all/+merge/147829

(do not edit description out of merge proposal)

[niemeyer, 2013-02-12 04:55:31]

Please take a look.

[jameinel, 2013-02-12 07:32:38]

LGTM

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

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode272
s3/multi.go:272: // the new part, or otherwise overwritten with the new content.
does partSize need to match exactly the pre-uploaded content? I don't see places
where that is checked here. Probably at least a doc comment about it.

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode282
s3/multi.go:282: if totalSize == 0 || err != nil {
What will this return if totalSize is 0? the return of ListParts? But doesn't
that violate the 'you should return nil, otherwise the caller gets an interface
object that contains nil but isn't nil'?

So I would probably split this:
if err != nil {
  return nil, err
} else if totalSize == 0 {
  return nil, nil
}

Though since you don't have a result object, wouldn't you want a non-nil err?
Otherwise you'll get a segfault accessing the result later.

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode310
s3/multi.go:310: part, err := m.putPart(current, section, partSize, md5b64)
As an aside, would there be any reason to do parallel uploads?

[dimitern, 2013-02-12 08:02:52]

LGTM, a few suggestions.

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

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode227
s3/multi.go:227: var listPartsMax = 1000
Is this an absolute limit of all parts per client connection?

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode282
s3/multi.go:282: if totalSize == 0 || err != nil {
On 2013/02/12 07:32:38, jameinel wrote:
> What will this return if totalSize is 0? the return of ListParts? But doesn't
> that violate the 'you should return nil, otherwise the caller gets an
interface
> object that contains nil but isn't nil'?
> 
> So I would probably split this:
> if err != nil {
>   return nil, err
> } else if totalSize == 0 {
>   return nil, nil
> }
> 
> Though since you don't have a result object, wouldn't you want a non-nil err?
> Otherwise you'll get a segfault accessing the result later. 

ISTM if totalSize == 0 it will return nil, nil, since there won't be an error.
But if it's not obvious, +1 on the suggestion.

https://codereview.appspot.com/7313081/diff/1/s3/multi_test.go
File s3/multi_test.go (right):

https://codereview.appspot.com/7313081/diff/1/s3/multi_test.go#newcode53
s3/multi_test.go:53: c.Assert(multi.UploadId, Matches, "JNbR_[A-Za-z0-9.]+QQ--")
Maybe move this in assertId()? This seems to be used multiple times, so it'll be
better to have it at one place, I think.

[niemeyer, 2013-02-12 14:41:56]

Thanks for the reviews.

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

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode227
s3/multi.go:227: var listPartsMax = 1000
On 2013/02/12 08:02:52, dimitern wrote:
> Is this an absolute limit of all parts per client connection?

No, this is a per-request limit, and 1000 is the default limit as well. It's
here as a variable just so we can test it. I'll add a comment.

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode272
s3/multi.go:272: // the new part, or otherwise overwritten with the new content.
On 2013/02/12 07:32:38, jameinel wrote:
> does partSize need to match exactly the pre-uploaded content? I don't see
places
> where that is checked here. Probably at least a doc comment about it.

It's actually explicitly checked, but that's a bit redundant with the above
statement. If the size changes, the checksum won't match. That said, given that
you felt the need to ask, it won't hurt to mention it explicitly.

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode282
s3/multi.go:282: if totalSize == 0 || err != nil {
On 2013/02/12 07:32:38, jameinel wrote:
> So I would probably split this:
> if err != nil {
>   return nil, err
> } else if totalSize == 0 {
>   return nil, nil
> }

Hmm.. that does exactly the same thing that the current version does.

> Though since you don't have a result object, wouldn't you want a non-nil err?
> Otherwise you'll get a segfault accessing the result later. 

That's not the case with slices. The most conventional way to represent an empty
slice is with a nil/zero value. len, append, etc, all work properly with them,
and code taking slices should behave properly as well, or may otherwise be
considered broken.

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode310
s3/multi.go:310: part, err := m.putPart(current, section, partSize, md5b64)
On 2013/02/12 07:32:38, jameinel wrote:
> As an aside, would there be any reason to do parallel uploads?

That's an interesting question. I guess we could probably get some speed up by
sending parts in parallel. It's a good topic to do some experimentation on.

[jameinel, 2013-02-13 06:01:26]

LGTM

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

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode282
s3/multi.go:282: if totalSize == 0 || err != nil {
That's fine. I wasn't thinking about it being a slice, it does seem correct
there.

The concern was stuff like: http://play.golang.org/p/ZcjGU2BgbC

But that doesn't apply here because err has type "error" rather than a custom
type that could be a nil pointer.

[rog, 2013-02-13 10:22:03]

really nice API.
a few suggestions below.

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

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode88
s3/multi.go:88: // inside b. If a multipart upload exists for key, it is
returned,
if we assume that a bucket implies its key, then i think the explanation can
become simpler. how about this, perhaps?

// Multi returns a multipart upload handler for the bucket.
// If one already exists, it is returned, otherwise
//  a new multipart upload is initiated with contType and perm.

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode229
s3/multi.go:229: // ListParts returns the list of previously uploaded parts in
m.
// ordered by part number.
?

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode269
s3/multi.go:269: // PutAll sends all of r via a multipart upload with parts no
large
s/large/larger/

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode273
s3/multi.go:273: // PutAll returns all the parts of m (reused or not).
nice interface.

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode279
s3/multi.go:279: reuse := 1   // Index of next old part to consider reusing.
I was a little confused here by the one-based indexing used for reuse here - it
makes it look like it's in the same units as current, but reuse really is an
index (into old) and current is a part number.

Also, current isn't really the index of the last good part handled, AFAICS -
it's the part number of the part we're about to consider putting.

I'd be inclined to do:

reuse := 0      // Index of next old part to consider reusing.
current := 1    // Part number of current part.

and adjust the use of reuse below.

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode282
s3/multi.go:282: if totalSize == 0 || err != nil {
On 2013/02/12 08:02:52, dimitern wrote:
> On 2013/02/12 07:32:38, jameinel wrote:
> > What will this return if totalSize is 0? the return of ListParts? But
doesn't
> > that violate the 'you should return nil, otherwise the caller gets an
> interface
> > object that contains nil but isn't nil'?
> > 
> > So I would probably split this:
> > if err != nil {
> >   return nil, err
> > } else if totalSize == 0 {
> >   return nil, nil
> > }
> > 
> > Though since you don't have a result object, wouldn't you want a non-nil
err?
> > Otherwise you'll get a segfault accessing the result later. 
> 
> ISTM if totalSize == 0 it will return nil, nil, since there won't be an error.
> But if it's not obvious, +1 on the suggestion.

i don't mind the way it's phrased here, but i would like to see a test case that
tests putting a zero-length file live (i *presume* you can read a file ok if
it's had no PUT requests, but it's only a presumption). perhaps i missed a
relevant test though.

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode316
s3/multi.go:316: }
the logic above is moderately intricate - i'd like to see some more test cases
to go with TestPutAllResume,
at least a case with a reused part in the middle of other non-reused parts, and
vice versa.

[niemeyer, 2013-02-13 10:38:35]

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

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode88
s3/multi.go:88: // inside b. If a multipart upload exists for key, it is
returned,
On 2013/02/13 10:22:03, rog wrote:
> if we assume that a bucket implies its key, then i think the explanation can
> become simpler. how about this, perhaps?
> 
> // Multi returns a multipart upload handler for the bucket.
> // If one already exists, it is returned, otherwise
> //  a new multipart upload is initiated with contType and perm.

A bucket doesn't imply its key. A bucket contains keys. What's described above
is not the same as what was documented in the method, or what the method does.

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode229
s3/multi.go:229: // ListParts returns the list of previously uploaded parts in
m.
On 2013/02/13 10:22:03, rog wrote:
> // ordered by part number.
> ?

Sounds good.

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode269
s3/multi.go:269: // PutAll sends all of r via a multipart upload with parts no
large
On 2013/02/13 10:22:03, rog wrote:
> s/large/larger/

Thanks.

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode279
s3/multi.go:279: reuse := 1   // Index of next old part to consider reusing.
On 2013/02/13 10:22:03, rog wrote:
> I was a little confused here by the one-based indexing used for reuse here -
it
> makes it look like it's in the same units as current, but reuse really is an
> index (into old) and current is a part number.
> 
> Also, current isn't really the index of the last good part handled, AFAICS -
> it's the part number of the part we're about to consider putting.
> 
> I'd be inclined to do:
> 
> reuse := 0      // Index of next old part to consider reusing.
> current := 1    // Part number of current part.
> 
> and adjust the use of reuse below.

Sounds good.

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode282
s3/multi.go:282: if totalSize == 0 || err != nil {
On 2013/02/13 10:22:03, rog wrote:
> On 2013/02/12 08:02:52, dimitern wrote:
> > On 2013/02/12 07:32:38, jameinel wrote:
> > > What will this return if totalSize is 0? the return of ListParts? But
> doesn't
> > > that violate the 'you should return nil, otherwise the caller gets an
> > interface
> > > object that contains nil but isn't nil'?
> > > 
> > > So I would probably split this:
> > > if err != nil {
> > >   return nil, err
> > > } else if totalSize == 0 {
> > >   return nil, nil
> > > }
> > > 
> > > Though since you don't have a result object, wouldn't you want a non-nil
> err?
> > > Otherwise you'll get a segfault accessing the result later. 
> > 
> > ISTM if totalSize == 0 it will return nil, nil, since there won't be an
error.
> > But if it's not obvious, +1 on the suggestion.
> 
> i don't mind the way it's phrased here, but i would like to see a test case
that
> tests putting a zero-length file live (i *presume* you can read a file ok if
> it's had no PUT requests, but it's only a presumption). perhaps i missed a
> relevant test though.

I don't think it matters whether that works or not in the server. That's an edge
case, and the server will happily report if it's an error to complete an upload
with no parts.

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode316
s3/multi.go:316: }
On 2013/02/13 10:22:03, rog wrote:
> the logic above is moderately intricate - i'd like to see some more test cases
> to go with TestPutAllResume,
> at least a case with a reused part in the middle of other non-reused parts,
and
> vice versa.

Sure, I can move the reused part to the middle.

[niemeyer, 2013-02-13 11:18:55]

Please take a look.

[rog, 2013-02-13 11:49:05]

LGTM modulo the below.

https://codereview.appspot.com/7313081/diff/8001/s3/multi.go
File s3/multi.go (right):

https://codereview.appspot.com/7313081/diff/8001/s3/multi.go#newcode276
s3/multi.go:276: // new content.
As discussed, perhaps a comment like:
// Multipart uploads of zero length files may not work.

https://codereview.appspot.com/7313081/diff/8001/s3/multi.go#newcode284
s3/multi.go:284: current := 1 // Index of latest good part handled.
This comment is still misleading. Current isn't an index in the same way that
reuse is, and at this point we haven't any good parts, so there's no "latest"
good part - current is the part number of the next part to handle.

[niemeyer, 2013-02-13 12:21:13]

Please take a look.

[niemeyer, 2013-02-13 12:30:29]

*** Submitted:

s3: Multi.PutAll for auto-chunking and resuming

Bucket.Multi method also added for trivial resume-or-init handling.

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