Stefan Sperling wrote on Wed, Sep 22, 2010 at 15:45:41 +0200:
> 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.
>
Unwedging the mirror is not bad; if some commit were replayed twice to
the mirror, /then/ I would be worried. :-)
Anyway, we could add that --use-pre-1.7-style-locking flag, or we could
make the warning message more strongly worded (say that it has been
known to corrupt/wedge mirror metadata), or probably a few other
options.
I don't feel strongly here; please feel free to jump in and implement
some saner handling for this case (a pre-1.7 target server).
> > > > 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.
>
Agreed. (but, and especially for this particular issue, I think you'll
do a better job than me of updating 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.
>
That function opens the RA session by itself, so the caller can't check
that. But the function could grow a boolean "bail if atomicity is
unavailable" parameter.
Received on 2010-09-22 16:12:43 CEST