You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2010/09/17 07:17:22 UTC
atomic-revprop mergeability, with 1.7 in mind
https://svn.apache.org/repos/asf/subversion/branches/atomic-revprop/BRANCH-README
The atomic-revprop branch, today, implements marshalling of the
OLD_VALUE_P parameter over all RA layers to the repos and fs layers.
(See svn_fs_change_rev_prop2() in trunk for a description of that parameter.)
The branch does not yet promise a specific error code will be returned if
the revprop change fails /due to the atomicity requirement/. Ensuring
a specific error code is the bulk of the remaining work on the branch;
implementing it requires teaching ra_dav how to marshal svn_error_t chains
over the wire.
In other words, the branch implements the API correctly and (from an
FS-oriented point of view) completely, and only the ability to signal to
the caller what sort of error condition occurred is absent. That
absence means a caller cannot tell[*] whether the propchange failure was
due to requesting an atomic change or due to some other reason.
Is it fine to merge the branch to trunk, given this state? Is the
branch, in its current state, fit for release (in 1.7), or is additional
work (possibly including the abovementioned errorcode work) required
prior to releasing it?
Daniel
(sleep-deprived)
[*] There is a kludge-y workaround: see is_atomicity_error() in
<ht...@daniel3.local>. It assumes
that the server did not localize(translate) the error message.
Re: atomic-revprop mergeability, with 1.7 in mind
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
C. Michael Pilato wrote on Fri, Sep 17, 2010 at 07:54:34 -0400:
> On 09/17/2010 03:17 AM, Daniel Shahaf wrote:
> > In other words, the branch implements the API correctly and (from an
> > FS-oriented point of view) completely, and only the ability to signal to
> > the caller what sort of error condition occurred is absent. That
> > absence means a caller cannot tell[*] whether the propchange failure was
> > due to requesting an atomic change or due to some other reason.
> >
> > Is it fine to merge the branch to trunk, given this state? Is the
> > branch, in its current state, fit for release (in 1.7), or is additional
> > work (possibly including the abovementioned errorcode work) required
> > prior to releasing it?
>
> Something tells me that a unique errorcode for atomicity failures will be
> useful. What will change on the client(s) behavior-wise if there is a new
> errorcode? I dunno. Maybe it can use the conflict resolution callback
> system to prompt the user to resolve what now could be conveyed as a
> conflict between their proposed revprop edit and the one that slipped in
> underneath them. Error string comparison (your noted workaround) is a
> non-starter -- let's not even speak of such things.
I agree that it's evil and ugly. That's why I haven't committed it (not
even to the branch).
>
> That said, it sounds like the code isn't a danger to the trunk. So the
> question really becomes, "Will you (or someone you know) see this task
> through to completion before 1.7 ships?" If you fully intend to do so, I
> don't see a problem with merging back to trunk ASAP (and updating
> roadmap.html not to point to the now-obsolete BRANCH-README).
>
I'll work on this and see this through to completion.
I'm not comfortable committing to dates on this work, since it's mostly
single-handed and I work on this between doing other things (which are
more important to me right now).
Do you think the featureset currently on the branch is releaseable? If
not, what do you think should be added to make it releaseable?
> --
> C. Michael Pilato <cm...@collab.net>
> CollabNet <> www.collab.net <> Distributed Development On Demand
>
Re: atomic-revprop mergeability, with 1.7 in mind
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 09/17/2010 03:17 AM, Daniel Shahaf wrote:
> In other words, the branch implements the API correctly and (from an
> FS-oriented point of view) completely, and only the ability to signal to
> the caller what sort of error condition occurred is absent. That
> absence means a caller cannot tell[*] whether the propchange failure was
> due to requesting an atomic change or due to some other reason.
>
> Is it fine to merge the branch to trunk, given this state? Is the
> branch, in its current state, fit for release (in 1.7), or is additional
> work (possibly including the abovementioned errorcode work) required
> prior to releasing it?
Something tells me that a unique errorcode for atomicity failures will be
useful. What will change on the client(s) behavior-wise if there is a new
errorcode? I dunno. Maybe it can use the conflict resolution callback
system to prompt the user to resolve what now could be conveyed as a
conflict between their proposed revprop edit and the one that slipped in
underneath them. Error string comparison (your noted workaround) is a
non-starter -- let's not even speak of such things.
That said, it sounds like the code isn't a danger to the trunk. So the
question really becomes, "Will you (or someone you know) see this task
through to completion before 1.7 ships?" If you fully intend to do so, I
don't see a problem with merging back to trunk ASAP (and updating
roadmap.html not to point to the now-obsolete BRANCH-README).
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: atomic-revprop mergeability, with 1.7 in mind
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Sperling wrote on Fri, Sep 17, 2010 at 12:03:20 +0200:
> On Fri, Sep 17, 2010 at 08:17:22AM +0100, Daniel Shahaf wrote:
> > https://svn.apache.org/repos/asf/subversion/branches/atomic-revprop/BRANCH-README
> >
> > The atomic-revprop branch, today, implements marshalling of the
> > OLD_VALUE_P parameter over all RA layers to the repos and fs layers.
> > (See svn_fs_change_rev_prop2() in trunk for a description of that parameter.)
> > The branch does not yet promise a specific error code will be returned if
> > the revprop change fails /due to the atomicity requirement/. Ensuring
> > a specific error code is the bulk of the remaining work on the branch;
> > implementing it requires teaching ra_dav how to marshal svn_error_t chains
> > over the wire.
> >
> > In other words, the branch implements the API correctly and (from an
> > FS-oriented point of view) completely, and only the ability to signal to
> > the caller what sort of error condition occurred is absent. That
> > absence means a caller cannot tell[*] whether the propchange failure was
> > due to requesting an atomic change or due to some other reason.
>
> Why do we need to return multiple errors when setting the revprop fails
> due to non-atomicity?
> Can't the server just select the proper error code from the chain
> and return that to the client? In a recent commit by cmike something
> similar was done (r997466).
>
ra_svn already marshals error chains, and I believe cmike's fix was for
the case where the error chain is headed by *two* SVN_ERR_RA_SVN_CMD_ERR
elements (rather than just one).
What I have to do now is make ra_dav preserve the error code in the
first place. You're correct, though, in observing that I need to
preserve just the topmost error code (as opposed to the whole chain).
> > Is it fine to merge the branch to trunk, given this state? Is the
> > branch, in its current state, fit for release (in 1.7), or is additional
> > work (possibly including the abovementioned errorcode work) required
> > prior to releasing it?
>
> I don't really like the error string comparison. I'd still be happy to
> see this in trunk, with comments indicating how the error handling should
> be done instead.
>
> Stefan
Re: atomic-revprop mergeability, with 1.7 in mind
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Sep 17, 2010 at 08:17:22AM +0100, Daniel Shahaf wrote:
> https://svn.apache.org/repos/asf/subversion/branches/atomic-revprop/BRANCH-README
>
> The atomic-revprop branch, today, implements marshalling of the
> OLD_VALUE_P parameter over all RA layers to the repos and fs layers.
> (See svn_fs_change_rev_prop2() in trunk for a description of that parameter.)
> The branch does not yet promise a specific error code will be returned if
> the revprop change fails /due to the atomicity requirement/. Ensuring
> a specific error code is the bulk of the remaining work on the branch;
> implementing it requires teaching ra_dav how to marshal svn_error_t chains
> over the wire.
>
> In other words, the branch implements the API correctly and (from an
> FS-oriented point of view) completely, and only the ability to signal to
> the caller what sort of error condition occurred is absent. That
> absence means a caller cannot tell[*] whether the propchange failure was
> due to requesting an atomic change or due to some other reason.
Why do we need to return multiple errors when setting the revprop fails
due to non-atomicity?
Can't the server just select the proper error code from the chain
and return that to the client? In a recent commit by cmike something
similar was done (r997466).
> Is it fine to merge the branch to trunk, given this state? Is the
> branch, in its current state, fit for release (in 1.7), or is additional
> work (possibly including the abovementioned errorcode work) required
> prior to releasing it?
I don't really like the error string comparison. I'd still be happy to
see this in trunk, with comments indicating how the error handling should
be done instead.
Stefan