You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Stein <gs...@gmail.com> on 2008/10/22 00:28:57 UTC
Re: svn commit: r33819 - branches/fsfs-pack/subversion/libsvn_fs_fs
On Tue, Oct 21, 2008 at 9:22 AM, <hw...@tigris.org> wrote:
>...
> @@ -6552,12 +6549,10 @@ pack_shard(const char *revs_dir,
> SVN_ERR(svn_io_file_open(&pack_file, pack_file_path,
> APR_WRITE | APR_CREATE | APR_BUFFERED,
> APR_OS_DEFAULT, pool));
> - SVN_ERR(svn_io_open_unique_file2(&manifest_file, &tmp_file_path,
> - manifest_path, ".manifest",
> - svn_io_file_del_on_pool_cleanup, pool));
> + SVN_ERR(svn_stream_open_unique(&pb.manifest_stream, &tmp_file_path, revs_dir,
> + svn_io_file_del_on_pool_cleanup, pool, pool));
> pb.next_offset = 0;
> pb.pack_stream = svn_stream_from_aprfile2(pack_file, FALSE, pool);
Do you need pack_file, or can you just svn_stream_open_writable() on
the pack_file_path?
(and/or as David suggested, do the initial packing on a temp file and
move into place)
Cheers,
-g
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r33819 - branches/fsfs-pack/subversion/libsvn_fs_fs
Posted by Greg Stein <gs...@gmail.com>.
On Tue, Oct 21, 2008 at 6:48 PM, Hyrum K. Wright
<hy...@mail.utexas.edu> wrote:
>...
>> (and/or as David suggested, do the initial packing on a temp file and
>> move into place)
>
> I'll have to go back through my logs of that conversation, but I don't really
> understand what that'd buy us. The usual reason to create the temp file is for
> atomicity, which we really don't need here because the atomic operation is to
> remove the shard directory. (There's probably some much more obvious
> explanation which I'm missing; feel free to enlighten me.)
Move the file *after* you know it is complete/good. If some code rolls
around and finds a file sitting there... is it good? bad? Who knows,
because you were working on constructing it in place.
But. I guess that presupposes you go for the file *first*. If you go
for the directory first, find it absent, then find a file... well,
that would work.
BUT, again, I think that you should shoot for the file first. Over the
lifetime of the repository, the directory will be absent many more
times than the file, so you want to save an I/O step and try the file
first. That means: don't make it available *unless* it is proper. And
that means construct elsewhere, and move when known-good (i.e. not for
atomicity, but for knowing it is complete and valid if it is visible).
Cheers,
-g
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r33819 - branches/fsfs-pack/subversion/libsvn_fs_fs
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Greg Stein wrote:
> On Tue, Oct 21, 2008 at 9:22 AM, <hw...@tigris.org> wrote:
>> ...
>> @@ -6552,12 +6549,10 @@ pack_shard(const char *revs_dir,
>> SVN_ERR(svn_io_file_open(&pack_file, pack_file_path,
>> APR_WRITE | APR_CREATE | APR_BUFFERED,
>> APR_OS_DEFAULT, pool));
>> - SVN_ERR(svn_io_open_unique_file2(&manifest_file, &tmp_file_path,
>> - manifest_path, ".manifest",
>> - svn_io_file_del_on_pool_cleanup, pool));
>> + SVN_ERR(svn_stream_open_unique(&pb.manifest_stream, &tmp_file_path, revs_dir,
>> + svn_io_file_del_on_pool_cleanup, pool, pool));
>> pb.next_offset = 0;
>> pb.pack_stream = svn_stream_from_aprfile2(pack_file, FALSE, pool);
>
> Do you need pack_file, or can you just svn_stream_open_writable() on
> the pack_file_path?
Ah, good point. See r33830.
> (and/or as David suggested, do the initial packing on a temp file and
> move into place)
I'll have to go back through my logs of that conversation, but I don't really
understand what that'd buy us. The usual reason to create the temp file is for
atomicity, which we really don't need here because the atomic operation is to
remove the shard directory. (There's probably some much more obvious
explanation which I'm missing; feel free to enlighten me.)
-Hyrum