New GridFS storage for environment

New GridFS storage for environment

A new ResourceStorage interface is defined to 
provide Put/Get/Remove functionality for a
specific storage implementation. A GridFS
implementation is also provided which stores
data in the "juju" database within a specified
namespace.

This is part of the Juju Internal Storage work.
The next step is to maintain a catalog of the
stored data to allow to dupe checks and querying.

https://code.launchpad.net/~wallyworld/juju-core/environ-gridfs-stroage/+merge/221315

(do not edit description out of merge proposal)

[wallyworld, 2014-05-29 05:41:22]

Please take a look.

[axw, 2014-05-29 06:05:04]

Looks pretty good, just a few questions/comments.

https://codereview.appspot.com/99580043/diff/1/state/storage/gridfs.go
File state/storage/gridfs.go (right):

https://codereview.appspot.com/99580043/diff/1/state/storage/gridfs.go#newcode56
state/storage/gridfs.go:56: if err != nil {
s/err/error/
Also, I think a more informative name would be "resultError" or "resultErr".

https://codereview.appspot.com/99580043/diff/1/state/storage/gridfs.go#newcode63
state/storage/gridfs.go:63: num, err := io.Copy(file, r)
I think you should be using CopyN here? Then you don't need to check num !=
length below, either.

https://codereview.appspot.com/99580043/diff/1/state/storage/interface.go
File state/storage/interface.go (right):

https://codereview.appspot.com/99580043/diff/1/state/storage/interface.go#new...
state/storage/interface.go:15: // Put writes data from the specified reader to
path and returns a checksum of the data written.
What's the deal with the checksum being returned? Shouldn't it be the
responsibility of the Put method to verify the operation if it wants to?

I'm mainly wondering why we need a ResourceStorage interface separate from the
Storage Interface, which is very similar.

[wallyworld, 2014-05-29 06:15:21]

Please take a look.

https://codereview.appspot.com/99580043/diff/1/state/storage/gridfs.go
File state/storage/gridfs.go (right):

https://codereview.appspot.com/99580043/diff/1/state/storage/gridfs.go#newcode56
state/storage/gridfs.go:56: if err != nil {
On 2014/05/29 06:05:04, axw wrote:
> s/err/error/
> Also, I think a more informative name would be "resultError" or "resultErr".

Bah, I mean to name the return param in the function signature "err".
I've fixed it.

https://codereview.appspot.com/99580043/diff/1/state/storage/gridfs.go#newcode63
state/storage/gridfs.go:63: num, err := io.Copy(file, r)
On 2014/05/29 06:05:04, axw wrote:
> I think you should be using CopyN here? Then you don't need to check num !=
> length below, either.

Done.

https://codereview.appspot.com/99580043/diff/1/state/storage/interface.go
File state/storage/interface.go (right):

https://codereview.appspot.com/99580043/diff/1/state/storage/interface.go#new...
state/storage/interface.go:15: // Put writes data from the specified reader to
path and returns a checksum of the data written.
On 2014/05/29 06:05:04, axw wrote:
> What's the deal with the checksum being returned? Shouldn't it be the
> responsibility of the Put method to verify the operation if it wants to?
> 
> I'm mainly wondering why we need a ResourceStorage interface separate from the
> Storage Interface, which is very similar.

checksum will be used later in subsequent work when we keep a catalog of stored
resources and want to do things like de-duping. 
Storage is being superseded by this new work but has to hang around while the
new stuff is done and then things ported across.

[axw, 2014-05-29 06:28:43]

LGTM

https://codereview.appspot.com/99580043/diff/20001/state/storage/gridfs.go
File state/storage/gridfs.go (right):

https://codereview.appspot.com/99580043/diff/20001/state/storage/gridfs.go#ne...
state/storage/gridfs.go:69: return file.MD5(), nil
Design says SHA256, but it's not clear to me if it actually matters (beyond
likelihood of collisions).

https://codereview.appspot.com/99580043/diff/20001/state/storage/interface.go
File state/storage/interface.go (right):

https://codereview.appspot.com/99580043/diff/20001/state/storage/interface.go...
state/storage/interface.go:15: // Put writes data from the specified reader to
path and returns a checksum of the data written.
Does the client need to know how to recompute the checksum, for verification? If
so, I think the specific type of hash should be specified here.

[wallyworld, 2014-05-29 06:46:38]

https://codereview.appspot.com/99580043/diff/20001/state/storage/gridfs.go
File state/storage/gridfs.go (right):

https://codereview.appspot.com/99580043/diff/20001/state/storage/gridfs.go#ne...
state/storage/gridfs.go:69: return file.MD5(), nil
On 2014/05/29 06:28:43, axw wrote:
> Design says SHA256, but it's not clear to me if it actually matters (beyond
> likelihood of collisions).

Yeah, MD5 is cheaper and provided natively by GridFS.Its just to detect dupes so
it's sufficient. IMO

https://codereview.appspot.com/99580043/diff/20001/state/storage/interface.go
File state/storage/interface.go (right):

https://codereview.appspot.com/99580043/diff/20001/state/storage/interface.go...
state/storage/interface.go:15: // Put writes data from the specified reader to
path and returns a checksum of the data written.
On 2014/05/29 06:28:43, axw wrote:
> Does the client need to know how to recompute the checksum, for verification?
If
> so, I think the specific type of hash should be specified here.

I'm going to add a helper method to calculate the checksum (or similar feature)
when the de-dupe logic is implemented.