You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2013/04/02 22:04:24 UTC
Re: [PATCH] Sleep for timestamps in the right places
Philip Martin wrote:
> Julian Foad <ju...@btopenworld.com> writes:
>> The attached patch attempts to improve our 'sleep for timestamps'
>> coverage, especially in error conditions where the WC has already been
>> changed and so we should sleep as well as returning an error. I just
>> happened to notice that we currently miss several such cases.
>>
>> The are several TODO notes in the log message. Before I go any
>> further with this, can anybody check I'm on the right track?
>
> Looks plausible.
>
>> + err = svn_client__checkout_internal(result_rev, URL, local_abspath,
>> + peg_revision, revision, depth,
>> + ignore_externals,
>> + allow_unver_obstructions, &sleep_here,
>> + ctx, pool);
>> + if (sleep_here)
>> + svn_io_sleep_for_timestamps(local_abspath, pool);
>
> Could we check use-commit-times and avoid the sleep? Either before
> sleeping or perhaps where we set sleep_here TRUE.
OK, thanks; I did that (inside update_internal()), and a few more things, and committed r1463721.
For the future, it seems to me that the tracking of whether we need to sleep should be done inside libsvn_wc, as only there do we really know whether we have made a change that relies on the timestamp. The current code is all in libsvn_client and can only assume that if it called something like svn_wc_revert or svn_wc_do_update then it will need to sleep, which is a crude assumption (not always true). We would also have the ability to store more detailed information there, such as the latest timestamp that we are relying on, so that we wouldn't have to sleep for 1 second starting from when we start to sleep but only starting from when the last WC update was, which might have been some time ago in some cases. But I'm not planning on doing any of that.
- Julian
Re: [PATCH] Sleep for timestamps in the right places
Posted by Julian Foad <ju...@btopenworld.com>.
C. Michael Pilato wrote:
> On 04/02/2013 04:04 PM, Julian Foad wrote:
>> For the future, it seems to me that the tracking of whether we need to
>> sleep should be done inside libsvn_wc, as only there do we really know
>> whether we have made a change that relies on the timestamp.
>
> This makes sense. File an ENHANCEMENT issue for it?
Good idea. <http://subversion.tigris.org/issues/show_bug.cgi?id=4342>, "Sleep for timestamps should be determined by libsvn_wc".
- Julian
Re: [PATCH] Sleep for timestamps in the right places
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 04/02/2013 04:04 PM, Julian Foad wrote:
> For the future, it seems to me that the tracking of whether we need to
> sleep should be done inside libsvn_wc, as only there do we really know
> whether we have made a change that relies on the timestamp.
This makes sense. File an ENHANCEMENT issue for it?
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
Re: FW: [PATCH] Sleep for timestamps in the right places
Posted by Julian Foad <ju...@btopenworld.com>.
Bert Huijben wrote:
> Philip Martin wrote:
>> "Bert Huijben" <be...@qqmail.nl> writes:
>>> And we use some of this time to see if you have a modern filesystem that
>>> stores more timestamp details than just seconds (by looking at the existing
>>> filesystem node passed to the function) and if you have such a filesystem
>>> we don't sleep at all.
>>
>> We still sleep on filesystems with high resolution but we switch to a
>> millisecond sleep instead of a second sleep.
>>
>> I still think that sleeping when use-commit-times=yes is odd. With
>> use-commit-times=no we know that timestamps set by the operation for are
>> in the past and the sleep takes account of the limited precision. With
>> use-commit-times=yes we simply don't know that timestamps are in the
>> past: they could be years in the past, slightly in the past, slightly in
>> the future or years in the future. Sleeping for 1s, or 1ms, in the hope
>> that it will solve problems with one tiny part of that range when it
>> could also create problems in another tiny part of that range is odd.
The rationale is that the "slightly in the past" scenario (where sleeping helps) is far more likely and worth supporting than the "slightly in the future" scenario (where sleeping hinders) -- for a longer discussion, see my reply to "Re: svn commit: r1464769 ..." a few minutes ago, <http://svn.haxx.se/dev/archive-2013-04/0131.shtml>
> While traversing the working copy we might update the record for files to
> mark/record them as unchanged, even though we didn't write them and
> didn't set the timestamp ourselves. (E.g. files that were merged. Or just
> updated externally)
>
> I don't really care about whether this case is interesting enough to
> perform/not perform the sleep. I try to avoid use-commit-times where
> possible.
>
> And optimizing for this nonstandard setting is not really interesting for
> me. My users usually won't see the sleep anyway.
Right. The whole "sleep for timestamps" functionality is as a rather crude "best effort" attempt to avoid problems, but doesn't address all scenarios. A better solution could be designed, as suggested in issue #4342 "Sleep for timestamps should be determined by libsvn_wc". My recent commits are just fixing up some inconsistencies and oversights in this existing support, and not attempting to address all scenarios.
- Julian
RE: FW: [PATCH] Sleep for timestamps in the right places
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: MARTIN PHILIP [mailto:codematters@ntlworld.com] On Behalf Of
> Philip Martin
> Sent: vrijdag 5 april 2013 14:09
> To: Bert Huijben
> Cc: dev@subversion.apache.org; julianfoad@btopenworld.com
> Subject: Re: FW: [PATCH] Sleep for timestamps in the right places
>
> "Bert Huijben" <be...@qqmail.nl> writes:
>
> > And we use some of this time to see if you have a modern filesystem that
> > stores more timestamp details than just seconds (by looking at the
existing
> > filesystem node passed to the function) and if you have such a
filesystem
> we
> > don't sleep at all.
>
> We still sleep on filesystems with high resolution but we switch to a
> millisecond sleep instead of a second sleep.
>
> I still think that sleeping when use-commit-times=yes is odd. With
> use-commit-times=no we know that timestamps set by the operation for are
> in the past and the sleep takes account of the limited precision. With
> use-commit-times=yes we simply don't know that timestamps are in the
> past: they could be years in the past, slightly in the past, slightly in
> the future or years in the future. Sleeping for 1s, or 1ms, in the hope
> that it will solve problems with one tiny part of that range when it
> could also create problems in another tiny part of that range is odd.
While traversing the working copy we might update the record for files to
mark/record them as unchanged, even though we didn't write them and didn't
set the timestamp ourselves. (E.g. files that were merged. Or just updated
externally)
I don't really care about whether this case is interesting enough to
perform/not perform the sleep. I try to avoid use-commit-times where
possible.
And optimizing for this nonstandard setting is not really interesting for
me. My users usually won't see the sleep anyway.
Bert
Re: FW: [PATCH] Sleep for timestamps in the right places
Posted by Philip Martin <ph...@wandisco.com>.
"Bert Huijben" <be...@qqmail.nl> writes:
> And we use some of this time to see if you have a modern filesystem that
> stores more timestamp details than just seconds (by looking at the existing
> filesystem node passed to the function) and if you have such a filesystem we
> don't sleep at all.
We still sleep on filesystems with high resolution but we switch to a
millisecond sleep instead of a second sleep.
I still think that sleeping when use-commit-times=yes is odd. With
use-commit-times=no we know that timestamps set by the operation for are
in the past and the sleep takes account of the limited precision. With
use-commit-times=yes we simply don't know that timestamps are in the
past: they could be years in the past, slightly in the past, slightly in
the future or years in the future. Sleeping for 1s, or 1ms, in the hope
that it will solve problems with one tiny part of that range when it
could also create problems in another tiny part of that range is odd.
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
FW: [PATCH] Sleep for timestamps in the right places
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Julian Foad [mailto:julianfoad@btopenworld.com]
> Sent: donderdag 4 april 2013 22:51
> To: Ben Reser
> Cc: Philip Martin; Subversion Development
> Subject: Re: [PATCH] Sleep for timestamps in the right places
>
> Ben Reser wrote:
> > On Tue, Apr 2, 2013 at 1:04 PM, Julian Foad wrote:
> >> Philip Martin wrote:
> >>> Could we check use-commit-times and avoid the sleep? Either before
> >>> sleeping or perhaps where we set sleep_here TRUE.
> >>
> >> OK, thanks; I did that (inside update_internal()), and a few more
things,
> >> and committed r1463721.
> >
> > I'm not sure this is such a great idea. If you use commit times it
> > may or may not be safe to skip sleeping. I discussed this a bit with
> > Philip on IRC today where he pointed out sleeping may also in some
> > cases may create problems as well.
> >
> > Commit timestamps may be in the past or the future.
> >
> > If they are less than the timestamp resolution of the file system in
> > the past then an immediate change to the file after the command exists
> > will not be visible to Subversion. This can be resolved by sleeping.
> >
> > If the timestamp is in the future then sleeping may bring you within
> > range of the timestamp resolution of the file system, resulting in the
> > same problem. The future timestamp problem can be dealt with by
> > keeping clocks in sync (NTP).
>
> Yes, we shan't attempt to handle the clock-skew problem at the
> moment. That is one of the issues that the enhancements suggested in
> <http://subversion.tigris.org/issues/show_bug.cgi?id=4342>, "Sleep for
> timestamps should be determined by libsvn_wc" could address.
>
> > It'd be easy to say "If you're doing the type of automation that's
> > bound to run into these sorts of problems don't use commit times."
> > However, I suspect there's times where you want to do both. As it is
> > right now if you're keeping your clocks in sync it should work okay.
> > But unless I'm missing something that's not the case after this
> > change.
>
> That's true, given that the current implementation doesn't look at the
> timestamps when deciding whether or how long to sleep. So the safe thing
> to do is reintroduce a sleep even if using commit times, for all
applicable
> operations (checkout, update, switch, revert).
>
> I'll commit this soon. (Running tests now.)
Note that calling svn_wc_text_modified_p2() while having a working copy lock
can update stored timestamps, so calling a status function while having a
working copy lock can have this effect.
Also note that we don't just sleep a second. We sleep until some fraction
after the next second starts. So by average this is about 0.6 seconds...
And we use some of this time to see if you have a modern filesystem that
stores more timestamp details than just seconds (by looking at the existing
filesystem node passed to the function) and if you have such a filesystem we
don't sleep at all.
With micro or sometimes nanosecond precision timestamps stored there is no
reason to sleep as an updated file is certain to have a different timestamp.
Bert
Re: [PATCH] Sleep for timestamps in the right places
Posted by Julian Foad <ju...@btopenworld.com>.
Julian Foad wrote:
>>[...] As it is
>> right now if you're keeping your clocks in sync it should work okay.
>> But unless I'm missing something that's not the case after this
>> change.
>
> That's true, given that the current implementation doesn't look at the
> timestamps when deciding whether or how long to sleep. So the safe thing to do
> is reintroduce a sleep even if using commit times, for all applicable operations
> (checkout, update, switch, revert).
>
> I'll commit this soon. (Running tests now.)
r1464769.
- Julian
Re: [PATCH] Sleep for timestamps in the right places
Posted by Julian Foad <ju...@btopenworld.com>.
Ben Reser wrote:
> On Tue, Apr 2, 2013 at 1:04 PM, Julian Foad wrote:
>> Philip Martin wrote:
>>> Could we check use-commit-times and avoid the sleep? Either before
>>> sleeping or perhaps where we set sleep_here TRUE.
>>
>> OK, thanks; I did that (inside update_internal()), and a few more things,
>> and committed r1463721.
>
> I'm not sure this is such a great idea. If you use commit times it
> may or may not be safe to skip sleeping. I discussed this a bit with
> Philip on IRC today where he pointed out sleeping may also in some
> cases may create problems as well.
>
> Commit timestamps may be in the past or the future.
>
> If they are less than the timestamp resolution of the file system in
> the past then an immediate change to the file after the command exists
> will not be visible to Subversion. This can be resolved by sleeping.
>
> If the timestamp is in the future then sleeping may bring you within
> range of the timestamp resolution of the file system, resulting in the
> same problem. The future timestamp problem can be dealt with by
> keeping clocks in sync (NTP).
Yes, we shan't attempt to handle the clock-skew problem at the moment. That is one of the issues that the enhancements suggested in <http://subversion.tigris.org/issues/show_bug.cgi?id=4342>, "Sleep for timestamps should be determined by libsvn_wc" could address.
> It'd be easy to say "If you're doing the type of automation that's
> bound to run into these sorts of problems don't use commit times."
> However, I suspect there's times where you want to do both. As it is
> right now if you're keeping your clocks in sync it should work okay.
> But unless I'm missing something that's not the case after this
> change.
That's true, given that the current implementation doesn't look at the timestamps when deciding whether or how long to sleep. So the safe thing to do is reintroduce a sleep even if using commit times, for all applicable operations (checkout, update, switch, revert).
I'll commit this soon. (Running tests now.)
- Julian
Re: [PATCH] Sleep for timestamps in the right places
Posted by Ben Reser <be...@reser.org>.
On Tue, Apr 2, 2013 at 1:04 PM, Julian Foad <ju...@btopenworld.com> wrote:
> Philip Martin wrote:
>> Could we check use-commit-times and avoid the sleep? Either before
>> sleeping or perhaps where we set sleep_here TRUE.
>
> OK, thanks; I did that (inside update_internal()), and a few more things, and committed r1463721.
I'm not sure this is such a great idea. If you use commit times it
may or may not be safe to skip sleeping. I discussed this a bit with
Philip on IRC today where he pointed out sleeping may also in some
cases may create problems as well.
Commit timestamps may be in the past or the future.
If they are less than the timestamp resolution of the file system in
the past then an immediate change to the file after the command exists
will not be visible to Subversion. This can be resolved by sleeping.
If the timestamp is in the future then sleeping may bring you within
range of the timestamp resolution of the file system, resulting in the
same problem. The future timestamp problem can be dealt with by
keeping clocks in sync (NTP).
It'd be easy to say "If you're doing the type of automation that's
bound to run into these sorts of problems don't use commit times."
However, I suspect there's times where you want to do both. As it is
right now if you're keeping your clocks in sync it should work okay.
But unless I'm missing something that's not the case after this
change.