Stefan Sperling wrote on Tue, Sep 21, 2010 at 17:01:13 +0200:
> On Tue, Sep 21, 2010 at 02:03:12PM -0000, danielsh_at_apache.org wrote:
> > Author: danielsh
> > Date: Tue Sep 21 14:03:12 2010
> > New Revision: 999421
> >
> > URL: http://svn.apache.org/viewvc?rev=999421&view=rev
> > Log:
> > On the atomic-revprops branch:
> >
> > In svnsync, grab the lock atomically, if possible.
> >
> > * subversion/svnsync/main.c
> > (is_atomicity_error): New helper.
> > (get_lock):
> > Take advantage of SVN_RA_CAPABILITY_ATOMIC_REVPROPS if present.
> > Grow output LOCK_STRING_P parameter.
> > (with_locked):
> > Remove the lock atomically.
> >
> > Modified:
> > subversion/branches/atomic-revprop/subversion/svnsync/main.c
> >
> > Modified: subversion/branches/atomic-revprop/subversion/svnsync/main.c
> > URL: http://svn.apache.org/viewvc/subversion/branches/atomic-revprop/subversion/svnsync/main.c?rev=999421&r1=999420&r2=999421&view=diff
> > ==============================================================================
> > --- subversion/branches/atomic-revprop/subversion/svnsync/main.c (original)
> > +++ subversion/branches/atomic-revprop/subversion/svnsync/main.c Tue Sep 21 14:03:12 2010
> > @@ -284,6 +284,14 @@ check_lib_versions(void)
> > }
> >
> >
> > +/* Does ERR mean "the current value of the revprop isn't equal to
> > + the *OLD_VALUE_P you gave me"?
> > + */
> > +static svn_boolean_t is_atomicity_error(svn_error_t *err)
> > +{
> > + return svn_error_has_cause(err, SVN_ERR_FS_PROP_BASEVALUE_MISMATCH);
> > +}
>
> Is this thin wrapper around svn_error_has_cause() really worth having?
>
Because it makes for a little more readability at the call site? </innocent voice>
Either way, is it worth the churn to remove it?
> > @@ -357,9 +375,27 @@ get_lock(svn_ra_session_t *session, apr_
> > }
> > else if (i < SVNSYNC_LOCK_RETRIES - 1)
> > {
> > + const svn_string_t *unset = NULL;
> > +
> > /* Except in the very last iteration, try to set the lock. */
> > - SVN_ERR(svn_ra_change_rev_prop(session, 0, SVNSYNC_PROP_LOCK,
> > - mylocktoken, subpool));
> > + err = svn_ra_change_rev_prop2(session, 0, SVNSYNC_PROP_LOCK,
> > + be_atomic ? &unset : NULL,
> > + mylocktoken, subpool);
> > +
> > + if (be_atomic && err && is_atomicity_error(err))
>
> Here you could call svn_error_has_cause() directly:
>
> if (be_atomic && err &&
> svn_error_has_cause(err, SVN_ERR_FS_PROP_BASEVALUE_MISMATCH))
>
> > + /* Someone else has the lock. Let's loop. */
> > + svn_error_clear(err);
> > + else if (be_atomic && err == SVN_NO_ERROR)
> > + /* We have the lock.
> > +
> > + However, for compatibility with concurrent svnsync's that don't
> > + support atomicity, loop anyway to double-check that they haven't
> > + overwritten our lock.
> > + */
>
> At this point we're racing with other svnsync instances that don't support
> proper locking and can corrupt the sync meta data anyway at any time.
> I think we can safely assume that users upgrade all their writing svnsync
> instances at once. And the server supports atomic revprop edits, so it's
> already been upgraded. Is more looping really worth it?
> We're essentially doing SVNSYNC_LOCK_RETRIES times more work than necessary.
>
The effect of this 'continue' is one additional svn_ra_rev_prop() call
and one additional strcmp(). I figured that the combined cost of these
two wasn't too significant overall, given the amount of RA work svnsync
(and get_lock()) do anyway.
But I don't feel strongly about this; I wouldn't mind seeing
s/continue;/return SVN_NO_ERROR;/ here.
> I'm very glad that this branch is virtually finished now :)
>
Thanks :-)
Received on 2010-09-21 18:45:49 CEST