[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] Make svn clients indicate their operation name to backend(right now only to DAV)

From: Julian Foad <julianfoad_at_btopenworld.com>
Date: Wed, 06 Jan 2010 15:48:51 +0000

Kamesh Jayachandran wrote:
> mod_dav_svn on the proxy should identify whether a given PROPFIND is for
> commit or *not*, we are not bothered about any of the other read
> operation intricasies.

As I understand it, the proxy was designed originally with some rules
that effectively said something like

  * The following operations are passed through to the master:
    MKACTIVITY ...

  * The following operations are handled by the proxy:
    REPORT OPTIONS ...

  * The following operations are handled by the proxy if referring to a
cached revision, else passed through to the master:
    PROPFIND GET ...

Now we discovered that this is too low-level, and in fact if a client
has done a commit (MKACTIVITY) it then needs to receive up-to-date info
when it subsequently does a PROPFIND, because it expects to be able to
see the results of its commit.

Can we define the new rules of proxying? I'm unfamiliar with it so my
guess below is probably way out. Please could you correct this attempt:

RULES OF PROXYING

  * The following operations are passed through to the master:
    MKACTIVITY ...

  * The following operations are handled by the proxy:
    REPORT OPTIONS ...

  * The following operations are handled by the proxy if referring to a
cached revision *AND IF THE CLIENT HAS NOT PERFORMED A COMMIT IN THIS
CONNECTION*, else passed through to the master:
    PROPFIND GET ...

And what is the theory behind these rules?

  * A client that has completed a commit needs to see the new state of
the master repository, in order to avoid errors like the one we are
fixing.

  * A client that has completed a commit must have started a commit and
therefore must have issued a MKACTIVITY.

  * A commit and any associated housekeeping tasks always occur within a
single connection. (? - doesn't sound right)

  * Only the PROPFIND needs this special handling, because it is the
only operation that can find out about a new head revision number. (?)

  * Only the MKACTIVITY needs to trigger this behaviour, because that is
the only operation by which this client can create a new head revision
number. (?)

My concern is whether the new rules are just papering over a crack or
whether they correctly take into account all the ways in which a client
might depend on accessing the newly created revision.

- Julian

> First I had a plan to persist the occurence of preceding 'MKACTIVITY' on
> the connection pool to classify subsequent PROPFIND on same connection
> as 'commit PROPFIND'.
>
> Justin was not happy about this as he was saying some bad old clients
> can make each requests in its own connection so this detection can be
> faulty for those poor clients.
>
> So this new SVN-ACTION request header came.
>
> The patch attached to the Philips response earlier applies 'SVN-ACTION'
> based detection and if it fails defaults to connection based detection.
>
> Once detected a PROPFIND as for commit we just proxy it.
>
> Hope this explains.
>
> With regards
> Kamesh Jayachandran
>
>
> On 01/06/2010 07:35 PM, Julian Foad wrote:
> > Kamesh,
> >
> > Can you explain your strategy for fixing the original "commit via
> > outdated proxy" issue, and why this change is part of that strategy?
> > Something about this approach doesn't feel right to me, as I don't see
> > how a single word can accurately convey the proxy semantics of a
> >
>
> Client never need to know anything about the proxy.
>
> > high-level command. I am wondering whether it is even possible to make
> > the proxy do what you want without having total control over the
> >
>
> We can, we have two detection strategies explained above which should
> catch most of the clients(even the ones which do *not* set SVN-ACTION
> header).
>
> > clients.
> >
> > (And if you do want to make some APIs take a "high-level operation" text
> > field, you need to specify what values are allowed and required - e.g.
> > does it have to be one of a fixed set of values, or any restrictions on
> > the length and what characters can be used, and whether the value has to
> > be known by the server, etc.)
> >
>
> Why not allow arbitrary value, let us say some fancy svn app can create
> custom workflow and give its own identifier say
> 'merge_aware_revision_graph etc.'
>
> As long as they are not calling 'commit' operation as a something else
> we are fine(Even then our backup logic would catch).
>
> With regards
> Kamesh Jayachandran
> > - Julian
> >
> >
> > Bert Huijben wrote:
> >
> >>
> >>> -----Original Message-----
> >>> From: Kamesh Jayachandran [mailto:kamesh_at_collab.net]
> >>> Sent: woensdag 6 januari 2010 14:00
> >>> To: dev_at_subversion.apache.org
> >>> Subject: [PATCH] Make svn clients indicate their operation name to
> >>> backend(right now only to DAV)
> >>>
> >>> Hi All,
> >>>
> >>> This patch is with respect to the original thread
> >>>
> >>> http://mail-archives.apache.org/mod_mbox/subversion-
> >>> dev/201001.mbox/browser
> >>>
> >>> Once this patch gets committed I can commit the mod_dav_svn change to
> >>> handle the original commit via outdated proxy issue.
> >>>
> >>> This Patch revs the following public APIs,
> >>>
> >>> 'svn_client_uuid_from_url', 'svn_client_open_ra_session' and
> >>> 'svn_ra_open3'.
> >>>
> >> I have a few high level questions about this patch:
> >>
> >> Why do you rev svn_client_uuid_from_url?
> >>
> >> I would think that that function is a high level API, so it would be an
> >> operation by itself.
> >>
> >> (looking at svn_client.h) What should I put in there as client that just
> >> needs the uuid or verify that the repository exists?
> >>
> >> "checking-uuid-for-visualization-to-my-great-users"?
> >>
> >> I don't think we should rev the svn_client_ API for this specific change
> >> here; especially since older clients will not pass anything anyway.
> >> libsvn_client should fill that high level operation for library users or the
> >> value is of no use on the server.
> >>
> >> And it should never be forwarded to master servers as the uuid is supposed
> >> to be constant per repository.
> >>
> >> (BTW. the api is new in 1.7, so it needs no revving at all)
> >>
> >>
> >> Then on to the rest of the patch:
> >>
> >>> For ra_neon and ra_serf layers it sets the http client header SVN-ACTION
> >>> with the concerned svn command name.
> >>>
> >>>
> >> If the operation is a plain string that can be set by any future client, how
> >> is the server to understand what the user wants? How can the server
> >> understand a new 'shelve' command we might add in Subversion 1.9?
> >>
> >> mod_dav_svn only knows RA operations and doesn't understand high level
> >> commands; we would have to add this knowledge.
> >>
> >>
> >> Shouldn't the individual RA operations tell whether the user needs access to
> >> the master or the slave?
> >>
> >> Thinking a bit further about that last issue... What if the session is
> >> reused for e.g. requests like 'svn info', 'svn update' and then a 'svn
> >> commit'.
> >> Our standard client libsvn_client can't do this, but other clients can
> >> certainly do that.
> >>
> >> There is nothing in the ra api that forbids using it that way, but just
> >> specifying a high level operation at open time doesn't tell enough about
> >> what the clients application intent is.
> >>
> >> Maybe we should just add a boolean to requests indicating whether to forward
> >> to a master? That seems like a much simpler solution, that we could possibly
> >> port back to older subversion releases.
> >>
> >> Bert
> >>
> >>
> >>> With& Without this patch mergeinfo_test-8 fails both over ra_neon and
> >>> ra_serf.
> >>>
> >>> If there are no objections I will commit this change in next 2 days.
> >>>
> >>> With regards
> >>> Kamesh Jayachandran
> >>>
> >>
> >
> >
>
Received on 2010-01-06 16:49:32 CET

This is an archived mail posted to the Subversion Dev mailing list.