On 22 May 2015 at 09:40, Branko Čibej <brane_at_wandisco.com> wrote:
> On 22.05.2015 00:09, Philip Martin wrote:
>> Ivan Zhakov <ivan_at_visualsvn.com> writes:
>>
>>> We considered adding FLUSH_TO_DISK flag to svn_stream_from_apr_file(),
>>> but decided to commit simple fix for now:
>>> 1. The current FSFS code uses explicit flush and this commit makes
>>> code consistent with other parts
>>> 2. Adding flag to svn_stream_from_apr_file() will require revving API
>>> etc, while this fix should be backported to 1.8.x and 1.9.x
>
> The API argument only applies to 1.8.x, which would need a completely
> different fix; not to 1.9.x, because it's not released yet.
Yes, we can introduce new API in 1.9.x but this automatically this fix
1.9.0 release blocker, while I wanted to avoid blocking 1.9.0 due this
fix since it's not regressing.
> The benefit
> of revising the API (which I tentatively support, see [1] below)
> outweighs the pain of having to write a different fix for 1.8.x.
>
>>> 3. It's unclear how FLUSH_TO_DISK flag should interact with DISOWN=TRUE.
>>>
>>> Taking in account all above we decided to commit simple fix for now
>>> and possibly implement FLUSH_TO_DISK flag later applying it to all
>>> FSFS code where needed.
>> Is it simple? It's possible that r1680819 is more complicated than
>> adding flush to the the stream.
>>
>> Exactly what the API should look like is bit unclear. Is there any need
>> to support disown=TRUE and flush=TRUE? When would Subversion use it?
>> If we create svn_stream_from_aprfile3 that takes two booleans then
>> perhaps we change the return type to allow an error for the impossible
>> disown=TRUE and flush=TRUE.
>
> Yup. There's no reason to try to support mutually conflicting arguments.
But how svn_stream_from_aprfile3() should handle disown=TRUE and
flush=TRUE: assertion, disown=TRUE as priority and ignore flush=TRUE?
I don't say we cannot find good solution for this, I just wanted to
make simple fix targeted for 1.8.x and 1.9.x and then improve all FSFS
regarding flushes.
>
>> All the flushing code goes in libsvn_subr
>
> Just one nit here: the flushing code would indeed be localized in
> libsvn_subr, however, the API change would propagate to some 100 places
> in the code [1], including lots of places in libsvn_fs_fs. That's unless
> we decide /not/ to deprecate svn_stream_from_aprfile2 in the 1.9.x
> series and use it in parallel with the prospective svn_stream_from_aprfile3.
>
Yep, revving svn_stream_from_aprfile2() would introduce changing all
callers. But I agree that we can keep svn_stream_from_aprfile2()
without deprecating.
> FWIW, I think in this case revving the API without deprecating the old
> one is justified. Another option would be to invent a different API name
> for the flushing stream, e.g., svn_stream_from_aprfile_flushed or some
> such. This way we'd also avoid the dilemma about disown by simply not
> having that flag in the new function.
>
There are two kinds of flushes:
1) flush all data buffered in stream implementation
2) ensure that data written to disk hardware
Extending stream API with svn_stream_flush() with (1) semantic seems
to be natural extension, but introducing (2) would be leak of
abstraction.
>> and the code change in FSFS is really small, something like:
>>
>> Index: subversion/libsvn_fs_fs/revprops.c
>> ===================================================================
>> --- subversion/libsvn_fs_fs/revprops.c (revision 1680818)
>> +++ subversion/libsvn_fs_fs/revprops.c (working copy)
>> @@ -874,7 +874,7 @@
>> new_filename->data,
>> pool),
>> APR_WRITE | APR_CREATE, APR_OS_DEFAULT, pool));
>> - *stream = svn_stream_from_aprfile2(file, FALSE, pool);
>> + SVN_ERR(svn_stream_from_aprfile3(stream, file, FALSE, TRUE, pool));
>>
>> return SVN_NO_ERROR;
>> }
>> @@ -1205,7 +1205,8 @@
>> SVN_ERR(svn__compress(uncompressed, compressed, compression_level));
>>
>> /* write the pack file content to disk */
>> - stream = svn_stream_from_aprfile2(pack_file, FALSE, scratch_pool);
>> + SVN_ERR(svn_stream_from_aprfile3(&stream, pack_file, FALSE, TRUE,
>> + scratch_pool));
>> SVN_ERR(svn_stream_write(stream, compressed->data, &compressed->len));
>> SVN_ERR(svn_stream_close(stream));
>>
>> A patch like that is possibly easier to review than r1680819.
>
> Quite a bit easier, yes.
>
Yes, this patch looks easier to review, the only problem that it's
incomplete. I'm attaching minimal working patch with
svn_stream_from_file3() against trunk_at_r1680818.
--
Ivan Zhakov
Received on 2015-05-22 14:36:28 CEST