edit_file patch

[Brad Fitzpatrick]

John,

Sorry for the slow review.

Overall, pretty good... few nits:

  -- the whitespace isn't consistent, either internally
     or with the rest of mogilefs.  get rid of trailing
     (end-of-line) whitespace, and make sure you're at
     4 space indents everywhere.  you seem to mix 4 and
     7 and 8 and sometimes not indenting some blocks.

  -- document, wherever users are likely to see it,
     that editing files is experimental and not what
     mogilefs is good at, and that we're assuming
     they know what they're doing.  probably document
     this in:

       +=head2 edit_file

  -- document also the requirements for the webserver?
     webdav, PUT, etc?  does your new test pass with
     default mogstored+perlbal?  or only your (which?)
     webserver?

  -- I don't like this internal method:

     + ($list, $fid, $dmid, $key) = $self->_get_paths($args);

     I don't like the calling conventions (returning random
     differently-typed things.. hard to read), but I mostly
     don't like it because it touched "sub cmd_get_paths",
     which is the most sensitive correctness/performance
     path in all of mogilefs.  I'd prefer if a new experimental
     thing like edit support stayed far away from that,
     even duplicating some code from now if you have to.
     Alternatively, a patch series which incrementally refactors
     it safely, but I thinking duplicating some code is okay
     for this.  Because I think this all might change a bit anyway,
     so let's not wed ourselves to get_paths more than we need to.

  -- document also above cmd_edit_file that this is experimental.

Clean those up, post another version here on the list, and mail me
separately about commit (http://danga.com/commit.bml) and when it's
good, you can just commit this stuff yourself.

Thanks!
Brad


On Tue, 24 Jul 2007, John Berthels wrote:

> Hi folks,
>
> As discussed here:
> http://lists.danga.com/pipermail/mogilefs/2007-June/001069.html,
> attached is a patch which implements an edit_file command.
>
> This patch:
>
> - adds a parallel method of wrapping filehandles,
> 'MogileFS::ClientHTTPFile', which is written in terms of LWP rather
> than directly to the socket layer.
>
> This implements a seekable read/write filehandle, with the same 'do
> create_close on close' behaviour as MogileFS::NewHTTPFile. It will
> fail across multiple URLs at 'tie' time.
>
> File operations are translated immediately into HTTP operations. This
> is beneficial for large files, since data is not accumulated in memory
> (but requires a store with support for partial PUT).
>
>
> - adds a read_file method to MogileFS::Client
>
> this is a wrapper which returns a readonly ClientHTTPFile around the
> results of get_paths
>
>
> - adds an edit_file method to MogileFS::Client (and a corresponding
> cmd_edit_file in MogileFS::Worker::Query on the back-end).
>
> The client edit_file returns a (read-write) ClientHTTPFile. The URL it
> works against is of a replica which has been disassociated from the
> key and associated with a new FID which is registered as a tempfile
> (with the same key).
>
> All being well, the existing create_close logic will delete all old
> replicas with this key when the filehandle returned from edit_file is
> closed, and the new file contents will replicate out.
>
> [This follows the implementation description in the message linked above.]
>
>
> I'd be interested in getting functionality like this into mainline
> MogileFS, so please let me know what you think of the approach and
> what sorts of changes you think might be needed to the patch before
> that could happen.
>
>
> regards,
>
> jb
>