On Wed, Sep 22, 2010 at 03:33:20PM +0200, Daniel Shahaf wrote:
> Stefan Sperling wrote on Wed, Sep 22, 2010 at 14:31:58 +0200:
> > On Wed, Sep 22, 2010 at 01:05:24PM +0200, Daniel Shahaf wrote:
> > > Daniel Shahaf wrote on Tue, Sep 21, 2010 at 18:50:56 +0200:
> > > > How about combining (1) and (3) --- that is, using the old locking mode
> > > > (with its known race condition) but print a warning that the mirror
> > > > server should be upgraded? That seems better than a fatal error for
> > > > a condition that needs to be fixed on the server end.
> > > >
> > >
> > > No replies, so I've implemented this in r999868. Let me know what you
> > > think :).
> > >
> > > The error message says "external locking"; does the svnbook define this term?
> >
> > Maybe use "disabled built-in locking", like the svnsync help sync says:
> >
> > --disable-locking : Disable built-in locking. Use of this option can
> > corrupt the mirror unless you ensure that no other
> > instance of svnsync is running concurrently.
> >
>
> The intention of the phrasing "external locking" is to remind users that
> they shouldn't pass --disable-locking unless they have some other,
> out-of-band mutex in use. (Which may be in their own head in the case of
> a single-user file://localhost mirror.) I'm afraid that just saying
> "disable built-in locking" will result in people passing --disable-locking
> without a second thought, which is much more likely to result in corruption.
>
> > > (so the error message could link to the workaround docs)
> >
> > There are no workaround docs, apart from some scattered mailing list
> > posts :(
> >
> > I'd prefer requiring people to pass either --disable-locking or
> > --use-pre-1.7-style-locking to write to mirrors using pre-1.7 servers.
>
> I see how an explicit --use-pre-1.7-style-locking would be justified if
> the race condition was easy to trigger and often caused corruptions, but
> I don't recall hearing much reports of the corruption issue on users@.
> Does the corruption issue (considering its severity and commonness)
> really warrant an explicit flag?
See Jon's comment made earlier about data integrity.
But, granted, there isn't any loss of versioned data when the race triggers.
The mirror needs to be unwedged by tweaking the svn:sync-* revprops at
revision 0. It will then resume operation.
> > > The docstring on trunk has always promised only a racy implementation.
> > > I'll just leave that as-is.
> >
> > But the Book hasn't. That is the problem.
> > Our users don't read the docstrings, and shouldn't have to.
> >
>
> Our users don't call svn_client_revprop_set2(); our API consumers do.
> And they should have warned *their* users about the raciness of that API
> since long before the branch.
We're also consumers of our APIs, so we should have warned our users :)
We've done so partly on the users@ list, if people happened to read
the relevant threads. But we should update the book, too. I'd say most
people rely on the book.
> One of those consumers is 'svn propedit --revprop'; are you suggesting we
> change the way 'svn propedit --revprop' works with pre-1.7 servers?
Hmmm... no. And that's actually a good point against having a command line
switch that is essentially there to allow svnsync to make revprop edits.
> On a more general issue, svn_client_revprop_set2()'s docstring promises
> that the change will be atomic if the server has the
> SVN_RA_CAPABILITY_ATOMIC_REVPROPS capability, but I'm not sure whether
> that condition is testable by svn_client_revprop_set2()'s caller?
Not sure. I don't know off-hand if the client has a way to query the
server's capabilities without doing a commit or prop edit.
Either way, I'm happy to see this problem fixed in 1.7.
Stefan
Received on 2010-09-22 15:46:29 CEST