On 28 May 2015 at 16:43, Philip Martin wrote:
> Ivan Zhakov writes:
>
>> I hope you're aware the original code has unbounded memory usage?
>
> No, could you explain? Is it a problem with the underlying stream code,
> or with the way it was used?
>
svn_stream_printf() allocates buffer for resulti
Ivan Zhakov writes:
> I hope you're aware the original code has unbounded memory usage?
No, could you explain? Is it a problem with the underlying stream code,
or with the way it was used?
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
On 28 May 2015 at 16:01, Philip Martin wrote:
> Ivan Zhakov writes:
>
>> Also I don't understand what do you mean "messy file-based code"? Imho
>> code using svn_stream_write() that require pointer to length is more
>> messy.
>
> "messy" is not my word but the reason I prefer the stream code is t
On 28.05.2015 15:01, Philip Martin wrote:
> Ivan Zhakov writes:
>
>> Also I don't understand what do you mean "messy file-based code"? Imho
>> code using svn_stream_write() that require pointer to length is more
>> messy.
> "messy" is not my word but the reason I prefer the stream code is that
> w
Ivan Zhakov writes:
> Also I don't understand what do you mean "messy file-based code"? Imho
> code using svn_stream_write() that require pointer to length is more
> messy.
"messy" is not my word but the reason I prefer the stream code is that
we have been moving towards it. Take this change:
Ivan Zhakov writes:
> I still think that flush should happen explicit when neded: it should
> not happen as flag when file/stream is opened. Someone who opens
> file/stream may doesn't know whether flush will be needed or not.
Can you give an example? The revprop code is exactly the opposite, t
On 28 May 2015 at 15:03, Branko Čibej wrote:
> On 28.05.2015 13:07, Julian Foad wrote:
>> Philip Martin wrote:
>>> I still prefer the stream patch I posted earlier, and it can be extended
>>> to support compressed streams. Something like:
>>>
>>> svn_error_t *
>>> svn_stream_flush_to_disk_on_clos
On 28 May 2015 at 14:56, Julian Foad wrote:
[...]
> Bert is pointing out that we shouldn't be trying to make a full flush
> to hardware after writing each file, as that's unreasonably slow and
> unnecessary.
>
> So we need to re-think the approach here.
>
The need for full flush in FSFS already d
On 28.05.2015 13:07, Julian Foad wrote:
> Philip Martin wrote:
>> I still prefer the stream patch I posted earlier, and it can be extended
>> to support compressed streams. Something like:
>>
>> svn_error_t *
>> svn_stream_flush_to_disk_on_close(svn_stream_t *stream)
>> {
>> if (stream->close_fn
Philip Martin wrote:
> The argument appears to be that we cannot design a perfect stream
> library and so we should write code using files instead.
No, that's not it at all.
> I'd prefer to
> accept limitations in our stream design and write a stream library that
> helps write the rest of our co
Julian Foad wrote:
> Philip Martin wrote:
>> I still prefer the stream patch I posted earlier, [...]
>>
>> That only allows flushing the stream on close but I do not see any need
>> at present to support flushing at arbitrary positions.
>
> The point about the generic stream API is you should alway
Julian Foad writes:
> The point about the generic stream API is you should always be able to
> define a new stream class that wraps a stream (examples: a 'tee'
> stream wraps one stream while copying to another; a checksumming
> stream, etc.).
>
> And you should always be able to use the wrapping
Philip Martin wrote:
> I still prefer the stream patch I posted earlier, and it can be extended
> to support compressed streams. Something like:
>
> svn_error_t *
> svn_stream_flush_to_disk_on_close(svn_stream_t *stream)
> {
> if (stream->close_fn == close_handler_apr)
> {
> svn_stream
Evgeny Kotkov writes:
> 1) We cannot really use svn_stream_flush(TRUE) + svn_stream_close() with
>a compressed stream that is chained over a file stream or a stream that we
>expect to be consistent with the underlying device. A compressed stream
>writes the final data chunk in close_
Ivan Zhakov wrote:
> Please find patch that adds disk flushes to FSFS packing code in attachment.
>
> I'm currently testing it and going to commit it later. This patch
> should also slightly improve packing performance as a bonus, since we
> can avoid APR buffered IO for some cases.
Note that you
On 27 May 2015 at 19:56, Ivan Zhakov wrote:
> On 27 May 2015 at 19:48, Philip Martin wrote:
>> Julian Foad writes:
>>
>>> Evgeny Kotkov wrote:
Philip, Julian, thank you for giving this patch a look.
As I said in my previous e-mails, I think that the committed fix is a
better
Philip Martin wrote:
> Julian Foad writes:
>> Evgeny Kotkov wrote:
>>> Philip, Julian, thank you for giving this patch a look.
>>>
>>> As I said in my previous e-mails, I think that the committed fix is a better
>>> choice here, as opposed to attempts to achieve the same behavior using the
>>> stre
On 27 May 2015 at 19:48, Philip Martin wrote:
> Julian Foad writes:
>
>> Evgeny Kotkov wrote:
>>> Philip, Julian, thank you for giving this patch a look.
>>>
>>> As I said in my previous e-mails, I think that the committed fix is a better
>>> choice here, as opposed to attempts to achieve the sam
Julian Foad writes:
> Evgeny Kotkov wrote:
>> Philip, Julian, thank you for giving this patch a look.
>>
>> As I said in my previous e-mails, I think that the committed fix is a better
>> choice here, as opposed to attempts to achieve the same behavior using the
>> stream API. [...]
>
> That soun
Evgeny Kotkov wrote:
> Philip, Julian, thank you for giving this patch a look.
>
> As I said in my previous e-mails, I think that the committed fix is a better
> choice here, as opposed to attempts to achieve the same behavior using the
> stream API. [...]
That sounds good to me -- leaving the cod
Philip Martin writes:
> Yes, that looks good.
[...]
Julian Foad writes:
> Just a couple of comments, from a quick read of the patch.
[...]
Philip, Julian, thank you for giving this patch a look.
As I said in my previous e-mails, I think that the committed fix is a better
choice here, as op
I (Julian Foad) wrote:
> Just a couple of comments, from a quick read of the patch.
>
> +/** Flushes all available internal buffers in a generic @a stream. If
> + * @sync is non-zero, it causes any buffered data to be written to the
> + * underlying device (for example, to the disk for a file stre
Philip Martin wrote:
> Evgeny Kotkov writes:
>> I sketched this option in the attached patch. With this patch applied, we
>> could rework the r1680819 fix like below. What do you think?
>
> Yes, that looks good.
Just a couple of comments, from a quick read of the patch.
+/** Flushes all availab
Evgeny Kotkov writes:
> I sketched this option in the attached patch. With this patch applied, we
> could rework the r1680819 fix like below. What do you think?
Yes, that looks good.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Philip Martin writes:
> Evgeny Kotkov writes:
>
>> Or is it just a gut feeling that we should be using streams here?
>
> We have been gradually moving our file-based code to stream-based code
> for years.
I believe that flushing files (instead of achieving the same behavior with
streams) is a b
Evgeny Kotkov writes:
> Or is it just a gut feeling that we should be using streams here?
We have been gradually moving our file-based code to stream-based code
for years.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Philip Martin writes:
> I like this patch better. It puts the flush into 2 extra places, 4 in
> total, in FSFS, and the corresponding 4 places in FSX. For 1.8 we would
> make the new API private: svn_stream__flush_to_disk_on_close.
>
> Index: subversion/include/svn_io.h
> ==
Ivan Zhakov writes:
> I intentionally kept packing code unchanged: it seems that now we
> don't use hardware flush when packing repository: so we need apply fix
> to all pack code, which is separate issue IMO.
Yes! I can confirm that with strace: we write pack/manifest files for
revisions witho
Ivan Zhakov writes:
> On 22 May 2015 at 11:50, Philip Martin wrote:
>>
>> Another approach would be to have a function to enable flushing directly
>> on the stream created by svn_stream_from_aprfile2() without creating a
>> new stream. This is probably the smallest/simplest patch. After
>> rev
On 22 May 2015 at 11:50, Philip Martin wrote:
> Branko Čibej writes:
>
>> 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
>>
Ivan Zhakov writes:
> 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@r1680818.
I like this patch better. It puts the flush into 2 extra places, 4 in
total, in FSFS, and the corresp
On 22 May 2015 at 09:40, Branko Čibej wrote:
> On 22.05.2015 00:09, Philip Martin wrote:
>> Ivan Zhakov 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 com
I mean we could leave the current 1.8.x backport proposal unchanged.
Alternatively, tweaking your patch to make the new API private for 1.8.x is
fine; probably even better as it's likely to make future backports easier.
On 22 May 2015 12:46 pm, "Philip Martin" wrote:
> Branko Čibej writes:
>
> >
Branko Čibej writes:
> On 22.05.2015 10:50, Philip Martin wrote:
>>
>> Another approach would be to have a function to enable flushing directly
>> on the stream created by svn_stream_from_aprfile2() without creating a
>> new stream. This is probably the smallest/simplest patch. After
>> reverti
On 22.05.2015 10:50, Philip Martin wrote:
> Branko Čibej writes:
>
>> 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
Branko Čibej writes:
> 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 diso
On 22.05.2015 00:09, Philip Martin wrote:
> Ivan Zhakov 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. A
Ivan Zhakov 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 requ
On 21 May 2015 at 16:25, Philip Martin wrote:
> i...@apache.org writes:
>
>> Author: ivan
>> Date: Thu May 21 11:00:43 2015
>> New Revision: 1680819
>>
>> URL: http://svn.apache.org/r1680819
>> Log:
>> Prevent a possible FSFS repository corruption with power or network disk
>> failures when changi
i...@apache.org writes:
> Author: ivan
> Date: Thu May 21 11:00:43 2015
> New Revision: 1680819
>
> URL: http://svn.apache.org/r1680819
> Log:
> Prevent a possible FSFS repository corruption with power or network disk
> failures when changing revision properties. Perform a hardware flush
> after w
40 matches
Mail list logo
