Code review comment for lp:~niemeyer/goamz/put-all

Revision history for this message
John A Meinel (jameinel) wrote :

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?

https://codereview.appspot.com/7313081/

« Back to merge proposal