Code review comment for lp:~niemeyer/goamz/put-all
Revision history for this message
| Gustavo Niemeyer (niemeyer) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
a60fb26
(Get the code!)
https:/
File s3/multi.go (right):
https:/
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:/
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:/
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:/
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:/
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:/
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.
https:/