Code review comment for lp:~niemeyer/goamz/put-all
Revision history for this message
| Roger Peppe (rogpeppe) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
a60fb26
(Get the code!)
really nice API.
a few suggestions below.
https:/
File s3/multi.go (right):
https:/
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:/
s3/multi.go:229: // ListParts returns the list of previously uploaded
parts in m.
// ordered by part number.
?
https:/
s3/multi.go:269: // PutAll sends all of r via a multipart upload with
parts no large
s/large/larger/
https:/
s3/multi.go:273: // PutAll returns all the parts of m (reused or not).
nice interface.
https:/
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:/
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:/
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.
https:/