You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Sperling <st...@elego.de> on 2010/09/21 16:25:10 UTC
Re: svnsync backwards-compatibility with older servers (Was: Re: svn
commit: r999421)
On Tue, Sep 21, 2010 at 05:07:49PM +0100, Jon Foster wrote:
> So... what do we do if a 1.7 svnsync connects to a <=1.6 mirror server?
> Some options are:
>
> 1) It works like 1.6 - i.e. it does buggy locking that works most of the
> time, then one day it corrupts the mirror repo.
>
> 2) It runs without even trying to do locking. This is likely to be more
> obvious to the sysadmin, so they might notice it in testing.
>
> 3) It errors out with an informative message. If the user has control
> of the mirror server, they can upgrade it to 1.7. Alternatively, if the
> user doesn't need locking (e.g. they have set up external locking), they
> can pass --disable-locking to svnsync.
>
>
> I really don't like option 1. I think Subversion should be reliable,
> and random corruption is not good. I'd like to get rid the old, buggy
> locking code.
>
> I'm not keen on option 2 either. Silently disabling a
> required-for-correctness feature seems like a really bad idea. And the
> locking has always been a documented feature (even if it's never
> worked).
>
> So I guess that leaves option 3. That's not fully backwards-compatible
> - you can't just drop in a 1.7 svnsync to replace a 1.6 svnsync, unless
> you update the server at the same time. But I feel that it's the least
> bad option.
I agree that option 3 is best. There's no point in hiding the problem
from the user in the name of backwards compatibility.
Daniel, this can easily be done on trunk, too, if you decide to merge
the branch back soon.
Stefan
Re: svnsync backwards-compatibility with older servers (Was: Re:
svn commit: r999421)
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 09/22/2010 12:43 PM, Jon Foster wrote:
> Daniel wrote:
>> Okay; then we should document (today) that when svnsync is used with
>> a 1.6 server, then the admin must ensure that two instances never start
>> at about the same time. (unless external locking is used)
>>
>> Not sure where this would go (the site, the book, the mailing lists..)
>
> We should fix the book. That's the canonical reference, and it
> currently tells everyone to use a buggy configuration.
File an issue in the svnbook tracker, and/or send a patch to the
svnbook-dev@red-bean.com list. I'll watch for it.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
RE: svnsync backwards-compatibility with older servers (Was: Re: svn
commit: r999421)
Posted by Jon Foster <Jo...@cabot.co.uk>.
Daniel wrote:
> Okay; then we should document (today) that when svnsync is used with
> a 1.6 server, then the admin must ensure that two instances never
start
> at about the same time. (unless external locking is used)
>
> Not sure where this would go (the site, the book, the mailing lists..)
We should fix the book. That's the canonical reference, and it
currently tells everyone to use a buggy configuration.
Kind regards,
Jon
**********************************************************************
This email and its attachments may be confidential and are intended solely for the use of the individual to whom it is addressed. Any views or opinions expressed are solely those of the author and do not necessarily represent those of Cabot Communications Ltd.
If you are not the intended recipient of this email and its attachments, you must take no action based upon them, nor must you copy or show them to anyone.
Cabot Communications Limited
Verona House, Filwood Road, Bristol BS16 3RY, UK
+44 (0) 1179584232
Co. Registered in England number 02817269
Please contact the sender if you believe you have received this email in error.
**********************************************************************
______________________________________________________________________
This email has been scanned by the MessageLabs Email Security System.
For more information please visit http://www.messagelabs.com/email
______________________________________________________________________
Re: svnsync backwards-compatibility with older servers (Was: Re:
svn commit: r999421)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
[ snipping tons of context ... ]
Jon Foster wrote on Wed, Sep 22, 2010 at 15:56:19 +0100:
> Daniel Shahaf wrote:
> > Unwedging the mirror is not bad; if some commit were replayed twice to
> > the mirror, /then/ I would be worried. :-)
>
> FYI, it *can* replay commits twice to the mirror.
>
> Real user bug report:
> http://svn.haxx.se/users/archive-2010-01/0184.shtml
>
> My analysis of that bug report:
> http://svn.haxx.se/users/archive-2010-01/0197.shtml
>
> To recover from that, the sysadmin would have to either delete
> and remirror, dump/load, or restore from backup. On a big
> repository, that's a lot of downtime.
>
Okay; then we should document (today) that when svnsync is used with
a 1.6 server, then the admin must ensure that two instances never start
at about the same time. (unless external locking is used)
Not sure where this would go (the site, the book, the mailing lists..)
> And from Daniel's earlier mail:
> > While here, there is a similar issue in svn_client_revprop_set2(). On
> > the branch, it tries to be atomic if possible; but on trunk, it has
> > always used a racy implementation. What do you think should be done
> > in that case?
>
> This is only used for "svn propedit" and GUI equivalents, right?
>
Yes.
> I think trying to be atomic if possible (i.e. what you do on the branch)
> is the right approach. This function existed before atomic-revprops and
> the race is even documented in it's docstring. Fixing the race is a
> nice improvement.
>
> The risk of hitting the race in svn_client_revprop_set2() is small, as
> it requires two humans to edit a revprop at the same time. The
> consequences are minor too - a possibly-incorrect revprop, which can
> be easily propedited again.
>
Indeed.
> I wouldn't worry about adding a "fail if not atomic" parameter. For
> the weird usecases where people care about atomicity, there's svn_ra.
RE: svnsync backwards-compatibility with older servers (Was: Re: svn
commit: r999421)
Posted by Jon Foster <Jo...@cabot.co.uk>.
Hi,
Daniel Shahaf wrote:
> 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 :).
I spent a while thinking about this. I think this is reasonable.
I think there will be people who use svnsync and have found that
the 1.6 locking has been working well enough for them. I now agree
we should let them keep using the old racy locking until they get
around to upgrading the mirror servers.
(I think "making upgrading to 1.7 easy" is probably the fastest
way to get everyone using fixed locking. And saying "svnsync 1.7
doesn't work against a 1.6 mirror unless you do some complicated
external locking stuff" makes the upgrade much harder).
Like Stefan, I was wondering whether this should be a command-line
option (either --allow-unsafe-locking or --safe-locking-only); I'm
not sure whether that would help or not. Printing the warning
every time might be enough.
> > > 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. :-)
FYI, it *can* replay commits twice to the mirror.
Real user bug report:
http://svn.haxx.se/users/archive-2010-01/0184.shtml
My analysis of that bug report:
http://svn.haxx.se/users/archive-2010-01/0197.shtml
To recover from that, the sysadmin would have to either delete
and remirror, dump/load, or restore from backup. On a big
repository, that's a lot of downtime.
And from Daniel's earlier mail:
> While here, there is a similar issue in svn_client_revprop_set2(). On
> the branch, it tries to be atomic if possible; but on trunk, it has
> always used a racy implementation. What do you think should be done
> in that case?
This is only used for "svn propedit" and GUI equivalents, right?
I think trying to be atomic if possible (i.e. what you do on the branch)
is the right approach. This function existed before atomic-revprops and
the race is even documented in it's docstring. Fixing the race is a
nice improvement.
The risk of hitting the race in svn_client_revprop_set2() is small, as
it requires two humans to edit a revprop at the same time. The
consequences are minor too - a possibly-incorrect revprop, which can
be easily propedited again.
I wouldn't worry about adding a "fail if not atomic" parameter. For
the weird usecases where people care about atomicity, there's svn_ra.
Kind regards,
Jon
**********************************************************************
This email and its attachments may be confidential and are intended solely for the use of the individual to whom it is addressed. Any views or opinions expressed are solely those of the author and do not necessarily represent those of Cabot Communications Ltd.
If you are not the intended recipient of this email and its attachments, you must take no action based upon them, nor must you copy or show them to anyone.
Cabot Communications Limited
Verona House, Filwood Road, Bristol BS16 3RY, UK
+44 (0) 1179584232
Co. Registered in England number 02817269
Please contact the sender if you believe you have received this email in error.
**********************************************************************
______________________________________________________________________
This email has been scanned by the MessageLabs Email Security System.
For more information please visit http://www.messagelabs.com/email
______________________________________________________________________
Re: svnsync backwards-compatibility with older servers (Was: Re:
svn commit: r999421)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
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.
Re: svnsync backwards-compatibility with older servers (Was: Re: svn
commit: r999421)
Posted by Stefan Sperling <st...@elego.de>.
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
Re: svnsync backwards-compatibility with older servers (Was: Re:
svn commit: r999421)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
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?
Either way, this is a SMOP to implement, since get_lock()'s caller has
a subcommand_baton_t available.
> This may be uncomfortable but will raise awareness of the issue.
> The error message should explain that the old servers will expose a
> race condition in svnsync's locking mechanism which can cause svnsync
> meta data to be corrupted on the mirror, and that for this reason, either
> the mirror server should be upgraded, or locking must be disabled entirely
> or the racy algorithm must be requested by the user. Users should also be
> made aware that they need to take care of preventing svnsync instances from
> writing to the mirror concurrently if the mirror doesn't run a 1.7 or
> later server.
>
> > > While here, there is a similar issue in svn_client_revprop_set2(). On
> > > the branch, it tries to be atomic if possible; but on trunk, it has
> > > always used a racy implementation. What do you think should be done
> > > in that case?
> >
> > 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.
One of those consumers is 'svn propedit --revprop'; are you suggesting we
change the way 'svn propedit --revprop' works with pre-1.7 servers?
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?
> Stefan
Re: svnsync backwards-compatibility with older servers (Was: Re: svn
commit: r999421)
Posted by Stefan Sperling <st...@elego.de>.
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.
> (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.
This may be uncomfortable but will raise awareness of the issue.
The error message should explain that the old servers will expose a
race condition in svnsync's locking mechanism which can cause svnsync
meta data to be corrupted on the mirror, and that for this reason, either
the mirror server should be upgraded, or locking must be disabled entirely
or the racy algorithm must be requested by the user. Users should also be
made aware that they need to take care of preventing svnsync instances from
writing to the mirror concurrently if the mirror doesn't run a 1.7 or
later server.
> > While here, there is a similar issue in svn_client_revprop_set2(). On
> > the branch, it tries to be atomic if possible; but on trunk, it has
> > always used a racy implementation. What do you think should be done
> > in that case?
>
> 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.
Stefan
Re: svnsync backwards-compatibility with older servers (Was: Re:
svn commit: r999421)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Shahaf wrote on Tue, Sep 21, 2010 at 18:50:56 +0200:
> Stefan Sperling wrote on Tue, Sep 21, 2010 at 18:25:10 +0200:
> > On Tue, Sep 21, 2010 at 05:07:49PM +0100, Jon Foster wrote:
> > > So... what do we do if a 1.7 svnsync connects to a <=1.6 mirror server?
> > > Some options are:
> > >
> > > 1) It works like 1.6 - i.e. it does buggy locking that works most of the
> > > time, then one day it corrupts the mirror repo.
> > >
> > > 2) It runs without even trying to do locking. This is likely to be more
> > > obvious to the sysadmin, so they might notice it in testing.
> > >
> > > 3) It errors out with an informative message. If the user has control
> > > of the mirror server, they can upgrade it to 1.7. Alternatively, if the
> > > user doesn't need locking (e.g. they have set up external locking), they
> > > can pass --disable-locking to svnsync.
> > >
> >
> > I agree that option 3 is best. There's no point in hiding the problem
> > from the user in the name of backwards compatibility.
> >
>
> 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?
(so the error message could link to the workaround docs)
> While here, there is a similar issue in svn_client_revprop_set2(). On
> the branch, it tries to be atomic if possible; but on trunk, it has
> always used a racy implementation. What do you think should be done
> in that case?
The docstring on trunk has always promised only a racy implementation.
I'll just leave that as-is.
Re: svnsync backwards-compatibility with older servers (Was: Re:
svn commit: r999421)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Sperling wrote on Tue, Sep 21, 2010 at 18:25:10 +0200:
> On Tue, Sep 21, 2010 at 05:07:49PM +0100, Jon Foster wrote:
> > So... what do we do if a 1.7 svnsync connects to a <=1.6 mirror server?
> > Some options are:
> >
Right now, the code uses option (1).
> > 1) It works like 1.6 - i.e. it does buggy locking that works most of the
> > time, then one day it corrupts the mirror repo.
> >
> > 2) It runs without even trying to do locking. This is likely to be more
> > obvious to the sysadmin, so they might notice it in testing.
> >
> > 3) It errors out with an informative message. If the user has control
> > of the mirror server, they can upgrade it to 1.7. Alternatively, if the
> > user doesn't need locking (e.g. they have set up external locking), they
> > can pass --disable-locking to svnsync.
> >
> >
> > I really don't like option 1. I think Subversion should be reliable,
> > and random corruption is not good. I'd like to get rid the old, buggy
> > locking code.
> >
> > I'm not keen on option 2 either. Silently disabling a
> > required-for-correctness feature seems like a really bad idea. And the
> > locking has always been a documented feature (even if it's never
> > worked).
> >
> > So I guess that leaves option 3. That's not fully backwards-compatible
> > - you can't just drop in a 1.7 svnsync to replace a 1.6 svnsync, unless
> > you update the server at the same time. But I feel that it's the least
> > bad option.
>
> I agree that option 3 is best. There's no point in hiding the problem
> from the user in the name of backwards compatibility.
>
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.
While here, there is a similar issue in svn_client_revprop_set2(). On
the branch, it tries to be atomic if possible; but on trunk, it has
always used a racy implementation. What do you think should be done
in that case?
> Daniel, this can easily be done on trunk, too, if you decide to merge
> the branch back soon.
>
> Stefan