brad at danga.com
Mon Aug 6 14:25:35 UTC 2007
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
-- document also the requirements for the webserver?
webdav, PUT, etc? does your new test pass with
default mogstored+perlbal? or only your (which?)
-- 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.
On Tue, 24 Jul 2007, John Berthels wrote:
> Hi folks,
> As discussed here:
> 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.
More information about the mogilefs