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


>whether it addresses any of the points Mike raised and in what ways,
>that sort of thing.

Hey forgot to answer this bit. Mike was suggesting to make use of 
'DAV:apply-to-version' in CHECKOUT rather than the problematic PROPFIND. I did 
not explore this bit yet.

With regards
Kamesh Jayachandran

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


>Sorry, Kamesh, that wasn't a very helpful question.

>I know the aim is to to fix the problem that occurs during a commit
>through a proxy server - that's the one-line overview. I know the patch
>adds new parameters to a bunch of functions and sets them to particular
>values and such like - I can see that by reading the patch. What I meant
>was please can you show us the log message, tell us what protocol
>changes it makes, what specific cases it fixes and what it doesn't, say
>whether it addresses any of the points Mike raised and in what ways,
>that sort of thing.

The patch is in concept complete(Yet to run through the testsuite and to 
justify few cases by exercising those code base, which I will do tomorrow) to 
make the indication of 'commit' operation via the new header 'SVN-COMMIT'. With 
this patch original problem gets fixed.

All the function signature changes just accepts a new boolean parameter 
'for_commit' which is set to TRUE only by commit.c:apply_revprops() while 
others set it to FALSE and hence no SVN-COMMIT header.

As it was a in concept patch I just did not write a log message, sorry for that.

With regards
Kamesh Jayachandran

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


>Hi Kamesh.

>What does this patch do?

It adds the magical header 'SVN-COMMIT' for PROPFIND calls associated with a 
commit operation over neon.

Server catches this magical header and decides whether to proxy or not.

As I mentioned already we need to do this only for neon as serf do not make 
PROPFIND and the POST it does already gets proxied.

With regards
Kamesh Jayachandran

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

2010-01-18 Thread Julian Foad

I (Julian Foad) wrote:
> Kamesh Jayachandran wrote:
> > Attached patch just fixes this at ra_neon layer alone(ra_serf do not 
> > need the fix at least on trunk, as it do not use the PROPFIND for this 
> > rather uses POST which do not suffer from this issue as it simply 
> > proxies to MASTER).
> 
> Hi Kamesh.
> 
> What does this patch do?

Sorry, Kamesh, that wasn't a very helpful question.

I know the aim is to to fix the problem that occurs during a commit
through a proxy server - that's the one-line overview. I know the patch
adds new parameters to a bunch of functions and sets them to particular
values and such like - I can see that by reading the patch. What I meant
was please can you show us the log message, tell us what protocol
changes it makes, what specific cases it fixes and what it doesn't, say
whether it addresses any of the points Mike raised and in what ways,
that sort of thing.

Thanks.
- Julian

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

2010-01-18 Thread Julian Foad

Kamesh Jayachandran wrote:
> Attached patch just fixes this at ra_neon layer alone(ra_serf do not 
> need the fix at least on trunk, as it do not use the PROPFIND for this 
> rather uses POST which do not suffer from this issue as it simply 
> proxies to MASTER).

Hi Kamesh.

What does this patch do?

- Julian

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


On 01/13/2010 05:07 PM, Kamesh Jayachandran wrote:

On 01/13/2010 12:08 AM, C. Michael Pilato wrote:

Kamesh Jayachandran wrote:

Hi All,

Last week I posted the patch to implement 'svn client' to identify the
svn operation they are about to do with a given ra_session.

Following thread has the detailed discussion.

http://mail-archives.apache.org/mod_mbox/subversion-dev/201001.mbox/%3c4b448959.1070...@collab.net%3e 





Below is the summary:

Concern/Suggestion 1:
Michael Pilato and Philip Martin were suggesting to tweak
libsvn_ra_(serf|neon) to detect the operation rather than making a
change across all layers from the simplicity point of view.

My answer to 1:
I feel it would be too hackish to tweak one general API inside these
modules to flag 'commit or write' operation to the server when the
solution I propose handles this in a transparent way.
I'm sorry, but did you say "transparent"?  What's transparent about 
bubbling
an RA-layer hack all the way up into the client?!  A "transparent" 
solution


This patch is *not* an RA layer hack rather a transparent generic 
feature so I see nothing wrong in propagating it to higher layers.


is one that preserves the existing transparency of the mirroring 
subsystem.


I do *not* like to expose the back-end internals to higher layers 
especially when it is not so common setup and not normally supposed to 
know(I mean why my client should know the repository it commits to is 
a mirror directly or indirectly).


  A "transparent" solution is one that doesn't allow ignorant 
third-party
consumers of the Subversion APIs to accidentally break their proxy 
setups
because they decide they wanted to pass "checkin" instead of "commit" 
as the

innocuous-appearing 'high_level_svn_operation' parameter.


Yes I agree. I was concerned about the *magical flag deep inside the 
code* for only one operation, now it looks like the only way to go.





There *must* be a better way to do this.

subversion/libsvn_ra_neon/commit.c:apply_revprops() has this comment:

   /* ### we should use DAV:apply-to-version on the CHECKOUT so we 
can skip

  ### retrieval of the baseline */

I looked a little bit into this.  IIUC, we can theoretically avoid the
problematic PROPFIND altogether by passing this special (yet
part-of-an-existing-standard) flag in the CHECKOUT request.  Currently,
though, mod_dav_svn doesn't handle the flag.  So there's still a 
server-side

change needed, but at least its one that would take us closer to better
WebDAV handling.  Maybe you could explore this option instead?



I will take a fresh look at this problem  and a independent patch in 
this way.


Thanks
With regards
Kamesh Jayachandran



Attached patch just fixes this at ra_neon layer alone(ra_serf do not 
need the fix at least on trunk, as it do not use the PROPFIND for this 
rather uses POST which do not suffer from this issue as it simply 
proxies to MASTER).


Have not tested the full test suite, I hope this is final unless I learn 
something new.


With regards
Kamesh Jayachandran



Index: subversion/libsvn_ra_neon/ra_neon.h
===
--- subversion/libsvn_ra_neon/ra_neon.h (revision 900319)
+++ subversion/libsvn_ra_neon/ra_neon.h (working copy)
@@ -447,6 +447,7 @@
  const char *url,
  int depth,
  const char *label,
+ svn_boolean_t for_commit,
  const ne_propname *which_props,
  apr_pool_t *pool);
 
@@ -455,6 +456,7 @@
   svn_ra_neon__session_t *sess,
   const char *url,
   const char *label,
+  svn_boolean_t for_commit,
   const ne_propname *which_props,
   apr_pool_t *pool);
 
@@ -491,6 +493,7 @@
 svn_ra_neon__session_t *sess,
 const char *url,
 const char *label,
+svn_boolean_t for_commit,
 const ne_propname *propname,
 apr_pool_t *pool);
 
Index: subversion/libsvn_ra_neon/props.c
===
--- subversion/libsvn_ra_neon/props.c   (revision 900319)
+++ subversion/libsvn_ra_neon/props.c   (working copy)
@@ -495,6 +495,7 @@
  const char *url,
  int depth,
  const char *label,
+ svn_boolean_t for_commit,

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

2010-01-13 Thread Peter Samuelson


[Kamesh Jayachandran]
> This patch is *not* an RA layer hack rather a transparent generic
> feature so I see nothing wrong in propagating it to higher layers.

Well, I agree with Mike, it is neither "transparent" nor "generic".
I know you dressed it up to look generic, but we all know that it is
specifically needed only for one particular RA backend, in one
particular configuration.  As you've said yourself, higher layers
should not have to know anything about proxying.

Ideally even the RA layer does nothing special, it is all handled by
mod_dav_svn, but if that's not possible, I'd like to see it only bubble
up to the RA layer.

Peter

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

2010-01-13 Thread Julian Foad

On Thu, 2010-01-07, Kamesh Jayachandran wrote:
> >sufficient, because, for example, "svn commit" followed immediately by
> >"svn up" should be guaranteed to update the WC to at least the revision
> >number of the commit, whereas with the proposed operation-name scheme I
> >think it would update to the last cached revision which can be older
> >than the commit.
> 
> This was the case I gave patch few months back which got a negative
> reaction as it was exposing the proxy details to the mod_dav_svn's
> update_editor code.
> 
> Now this patch does not affect its behaviour.
> 
> wc_from_slave$svnversion
> 41
> wc_from_slave$modify something
> 
> wc_from_slave$svn ci -m "commit"
> Committed 42
> wc_from_slave$svnversion
> 41:42
> 
> wc_from_slave$svn up
> 
> With this patch the 'svn up' scenario works as follows,
> Get the HEAD from the server via PROPFIND which would be unaffected
> with our patch as it is not for the 'commit' operation, i.e no
> preceding MKACTIVITY or SVN-ACTION != commit so *no* proxying to
> Master.

That means the 'svn up' believes that HEAD==41 and so it runs the update
editor for 'r41' with one of the child being in r42, and that gets the
update editor confused. Yes?

That is part of my concern. I think we should try to find a solution to
the commit problem that also solves this case, because then I think we
will have solved a much more general problem, not just a specific case
that is related to the way the commit operation currently works.

I suggested some other ways in an earlier email,
, 
namely a client unique id or the client telling the server its last known head 
revision number. What do you think about those? (Never mind for the moment how 
to generate a unique id; let's first decide if that would solve the problem.)

- Julian

> Without this patch the 'svn up' scenario works as follows,
> Get the HEAD from the server via PROPFIND which is not proxyed to
> Master.
> 
> After getting the HEAD *from*(not Via) the Slave it runs the update
> editor for 'r41' with one of the child being in r42 to get update
> editor confused(That is what my patch was handling and gave a
> meaningful/less frightening message).
> 
> With regards
> Kamesh Jayachandran

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

2010-01-13 Thread Kamesh Jayachandran

On 01/13/2010 12:08 AM, C. Michael Pilato wrote:

Kamesh Jayachandran wrote:

Hi All,

Last week I posted the patch to implement 'svn client' to identify the
svn operation they are about to do with a given ra_session.

Following thread has the detailed discussion.

http://mail-archives.apache.org/mod_mbox/subversion-dev/201001.mbox/%3c4b448959.1070...@collab.net%3e

Below is the summary:

Concern/Suggestion 1:
Michael Pilato and Philip Martin were suggesting to tweak
libsvn_ra_(serf|neon) to detect the operation rather than making a
change across all layers from the simplicity point of view.

My answer to 1:
I feel it would be too hackish to tweak one general API inside these
modules to flag 'commit or write' operation to the server when the
solution I propose handles this in a transparent way.

I'm sorry, but did you say "transparent"? What's transparent about bubbling
an RA-layer hack all the way up into the client?! A "transparent" solution

This patch is *not* an RA layer hack rather a transparent generic
feature so I see nothing wrong in propagating it to higher layers.

is one that preserves the existing transparency of the mirroring subsystem.

I do *not* like to expose the back-end internals to higher layers
especially when it is not so common setup and not normally supposed to
know(I mean why my client should know the repository it commits to is a
mirror directly or indirectly).

A "transparent" solution is one that doesn't allow ignorant third-party
consumers of the Subversion APIs to accidentally break their proxy setups
because they decide they wanted to pass "checkin" instead of "commit" as the
innocuous-appearing 'high_level_svn_operation' parameter.

Yes I agree. I was concerned about the *magical flag deep inside the
code* for only one operation, now it looks like the only way to go.

There *must* be a better way to do this.

subversion/libsvn_ra_neon/commit.c:apply_revprops() has this comment:

/* ### we should use DAV:apply-to-version on the CHECKOUT so we can skip
### retrieval of the baseline */

I looked a little bit into this. IIUC, we can theoretically avoid the
problematic PROPFIND altogether by passing this special (yet
part-of-an-existing-standard) flag in the CHECKOUT request. Currently,
though, mod_dav_svn doesn't handle the flag. So there's still a server-side
change needed, but at least its one that would take us closer to better
WebDAV handling. Maybe you could explore this option instead?

I will take a fresh look at this problem and a independent patch in
this way.

Thanks
With regards
Kamesh Jayachandran

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

2010-01-12 Thread C. Michael Pilato

Kamesh Jayachandran wrote:
> Hi All,
> 
> Last week I posted the patch to implement 'svn client' to identify the
> svn operation they are about to do with a given ra_session.
> 
> Following thread has the detailed discussion.
> 
> http://mail-archives.apache.org/mod_mbox/subversion-dev/201001.mbox/%3c4b448959.1070...@collab.net%3e
> 
> 
> 
> Below is the summary:
> 
> Concern/Suggestion 1:
> Michael Pilato and Philip Martin were suggesting to tweak
> libsvn_ra_(serf|neon) to detect the operation rather than making a
> change across all layers from the simplicity point of view.
> 
> My answer to 1:
> I feel it would be too hackish to tweak one general API inside these
> modules to flag 'commit or write' operation to the server when the
> solution I propose handles this in a transparent way.

I'm sorry, but did you say "transparent"?  What's transparent about bubbling
an RA-layer hack all the way up into the client?!  A "transparent" solution
is one that preserves the existing transparency of the mirroring subsystem.
 A "transparent" solution is one that doesn't allow ignorant third-party
consumers of the Subversion APIs to accidentally break their proxy setups
because they decide they wanted to pass "checkin" instead of "commit" as the
innocuous-appearing 'high_level_svn_operation' parameter.

There *must* be a better way to do this.

subversion/libsvn_ra_neon/commit.c:apply_revprops() has this comment:

  /* ### we should use DAV:apply-to-version on the CHECKOUT so we can skip
 ### retrieval of the baseline */

I looked a little bit into this.  IIUC, we can theoretically avoid the
problematic PROPFIND altogether by passing this special (yet
part-of-an-existing-standard) flag in the CHECKOUT request.  Currently,
though, mod_dav_svn doesn't handle the flag.  So there's still a server-side
change needed, but at least its one that would take us closer to better
WebDAV handling.  Maybe you could explore this option instead?

-- 
C. Michael Pilato 
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand

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

2010-01-08 Thread Kamesh Jayachandran


On 01/06/2010 09:09 PM, C. Michael Pilato wrote:

Philip Martin wrote:
   

Kamesh Jayachandran  writes:

 

This patch is with respect to the original thread

http://mail-archives.apache.org/mod_mbox/subversion-dev/201001.mbox/browser
   

This one I suppose:

http://mail-archives.apache.org/mod_mbox/subversion-dev/201001.mbox/<4b41f1bd.8090...@collab.net>

It includes:

"We can proxy this request to the Master but we *should not* do
 that if it is for read operation."
 

With all due respect, the proposed solution looks enormous compared to the
size of the problem.   Does the original problem exist in HTTPv2?  At a
minimum, could the ra_dav providers not annotate the PROPFIND as
"dont-proxy-this" without even touching the RA (and higher) APIs?

   


May be I can tweak 'svn_ra_neon__get_one_prop' which I believe the one 
that makes the problematic PROPFIND call.


Though the quantum of change would be relatively smaller than my 
original patch, I do *not* like it as it looks too secretive to set one 
custom flag deep inside the code for one special case.


My patch has the additional(tangential to its original intension) 
benefit of 'administrative control of activity based on operation names'.


With regards
Kamesh Jayachandran

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

2010-01-07 Thread Kamesh Jayachandran


On 01/07/2010 07:53 PM, C. Michael Pilato wrote:

Kamesh Jayachandran wrote:
   
 

With all due respect, the proposed solution looks enormous compared to
the
size of the problem.   Does the original problem exist in HTTPv2?  At a
minimum, could the ra_dav providers not annotate the PROPFIND as
"dont-proxy-this" without even touching the RA (and higher) APIs?


   

I *think* neon still uses the old protocol i.e do not make POST requests
so suffers from original PROPFIND problem and hence need our fix.

*serf* uses 'POST' request to get the baseline revision, 'POST' request
is proxied to MASTER so it does not suffer from this problem.
 

Yes, that's correct.  Neon only grew partial HTTPv2 support, mostly for the
read operations, but definitely *not* for commits.  If switching to POST
will fix this, then maybe that's the least invasive approach -- you (or
someone) simply needs to get Neon doing HTTPv2 for commit stuffs.

Another option (and I've not researched this, so it might be bogus) is to
look into the target of that problematic PROPFIND.  I mean, is the default
VCC the only resource that can answer the query?  If not, could perhaps some
obviously-commit-related resource answer it (something in the activity,
perhaps)?  Do you happen to know exactly what is being asked for in that
PROPFIND today?

   


Request look like this,
PROPFIND /svn2/demo/!svn/vcc/default HTTP/1.1^M









With regards
Kamesh Jayachandran

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

2010-01-07 Thread C. Michael Pilato

Kamesh Jayachandran wrote:
> 
>> With all due respect, the proposed solution looks enormous compared to
>> the
>> size of the problem.   Does the original problem exist in HTTPv2?  At a
>> minimum, could the ra_dav providers not annotate the PROPFIND as
>> "dont-proxy-this" without even touching the RA (and higher) APIs?
>>
>>
> 
> I *think* neon still uses the old protocol i.e do not make POST requests
> so suffers from original PROPFIND problem and hence need our fix.
> 
> *serf* uses 'POST' request to get the baseline revision, 'POST' request
> is proxied to MASTER so it does not suffer from this problem.

Yes, that's correct.  Neon only grew partial HTTPv2 support, mostly for the
read operations, but definitely *not* for commits.  If switching to POST
will fix this, then maybe that's the least invasive approach -- you (or
someone) simply needs to get Neon doing HTTPv2 for commit stuffs.

Another option (and I've not researched this, so it might be bogus) is to
look into the target of that problematic PROPFIND.  I mean, is the default
VCC the only resource that can answer the query?  If not, could perhaps some
obviously-commit-related resource answer it (something in the activity,
perhaps)?  Do you happen to know exactly what is being asked for in that
PROPFIND today?

-- 
C. Michael Pilato 
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand

signature.asc
Description: OpenPGP digital signature

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

2010-01-07 Thread Kamesh Jayachandran




With all due respect, the proposed solution looks enormous compared to the
size of the problem.   Does the original problem exist in HTTPv2?  At a
minimum, could the ra_dav providers not annotate the PROPFIND as
"dont-proxy-this" without even touching the RA (and higher) APIs?

   


I *think* neon still uses the old protocol i.e do not make POST requests 
so suffers from original PROPFIND problem and hence need our fix.


*serf* uses 'POST' request to get the baseline revision, 'POST' request 
is proxied to MASTER so it does not suffer from this problem.


With regards
Kamesh Jayachandran

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

>> NO, Client is unaware of existence of master or proxying scheme at all.

>The client doesn't need to know about the master, it just marks the
>PROPFINDs associated with MKACTIVITYs as special.

May be we can achieve this via some ra_session's priv data structure. I just 
tend to think what if we need to handle something else like this on the backend 
especially on all 'ra' layers. Will we again do some other trick on a case by 
case basis?

With regards
Kamesh Jayachandran

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

"Kamesh Jayachandran"  writes:

>>So, on the client side libsvn_ra_neon and libsvn_ra_serf could track
>>whether the PROPFIND needs to go to the master or the slave and then
>>add a suitable header to the request.  libsvn_client doesn't need to
>>change (which is what I think Justin and Mike are suggesting).
>
> NO, Client is unaware of existence of master or proxying scheme at all.

The client doesn't need to know about the master, it just marks the
PROPFINDs associated with MKACTIVITYs as special.

> Only the proxy knows the existence of Master.

If the server is a proxy it looks for the special marker, if it's not
a proxy it ignores it.

-- 
Philip

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

>sufficient, because, for example, "svn commit" followed immediately by
>"svn up" should be guaranteed to update the WC to at least the revision
>number of the commit, whereas with the proposed operation-name scheme I
>think it would update to the last cached revision which can be older
>than the commit.


This was the case I gave patch few months back which got a negative reaction as 
it was exposing the proxy details to the mod_dav_svn's update_editor code.

Now this patch does not affect its behaviour.

wc_from_slave$svnversion
41
wc_from_slave$modify something

wc_from_slave$svn ci -m "commit"
Committed 42
wc_from_slave$svnversion
41:42

wc_from_slave$svn up

With this patch the 'svn up' scenario works as follows,
Get the HEAD from the server via PROPFIND which would be unaffected with our 
patch as it is not for the 'commit' operation, i.e no preceding MKACTIVITY or 
SVN-ACTION != commit so *no* proxying to Master.

Without this patch the 'svn up' scenario works as follows,
Get the HEAD from the server via PROPFIND which is not proxyed to Master.

After getting the HEAD *from*(not Via) the Slave it runs the update editor for 
'r41' with one of the child being in r42 to get update editor confused(That is 
what my patch was handling and gave a meaningful/less frightening message).

With regards
Kamesh Jayachandran

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


>>>Let me see if I've got this right.  There is a MKACTIVITY that goes to
>>>the master, then a PROPFIND that goes to the slave, and finally a
>>>CHECKOUT that goes to master.  The MKACTIVITY and CHECKOUT go to the
>>>master because they are obviously write operations, the PROPFIND goes
>>>to the slave because it cannot be distinguished from a read-only
>>>operation that doesn't need to access the master.  The PROPFIND
>>>returns out-of-date info that causes the CHECKOUT to fail.
>>
>> Yes you got it correct.

>So, on the client side libsvn_ra_neon and libsvn_ra_serf could track
>whether the PROPFIND needs to go to the master or the slave and then
>add a suitable header to the request.  libsvn_client doesn't need to
>change (which is what I think Justin and Mike are suggesting).

NO, Client is unaware of existence of master or proxying scheme at all.

Only the proxy knows the existence of Master.

With regards
Kamesh Jayachandran

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

Julian,

>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 ...

Add MERGE, LOCK, UNLOCK and any request having '!svn' in its URI.

>  * 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.

MKACTIVITY is *not* commit though it is a starting point of any 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 ...

Add MERGE, LOCK, UNLOCK and any request having '!svn' in its URI.

>  * 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.

I think you misunderstood the problem, the error we have observed occurs even 
before the commit completes rather it occurs during the start phase of the 
commit.

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

commit is not complete here.


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

It can occur over multiple connections though most of the time it may happen on 
the same connection. If it happens on the same connection we do not need 
'SVN-ACTION' header at all as we can belive the connection pool, unfortunately 
it is not.


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

Not sure of other operations :).

>  * 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. (?)


I think 'MERGE' creates the head revision, we use MKACTIVITY as identification 
of commit in progress.

With regards
Kamesh Jayachandran

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

"Kamesh Jayachandran"  writes:

>>Let me see if I've got this right.  There is a MKACTIVITY that goes to
>>the master, then a PROPFIND that goes to the slave, and finally a
>>CHECKOUT that goes to master.  The MKACTIVITY and CHECKOUT go to the
>>master because they are obviously write operations, the PROPFIND goes
>>to the slave because it cannot be distinguished from a read-only
>>operation that doesn't need to access the master.  The PROPFIND
>>returns out-of-date info that causes the CHECKOUT to fail.
>
> Yes you got it correct.

So, on the client side libsvn_ra_neon and libsvn_ra_serf could track
whether the PROPFIND needs to go to the master or the slave and then
add a suitable header to the request.  libsvn_client doesn't need to
change (which is what I think Justin and Mike are suggesting).

-- 
Philip

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

>With all due respect, the proposed solution looks enormous compared to the
>size of the problem.   

It looks enormous for the sheer volume of signature changes! apart from the 
code change is minimal.

I tend to imagine(may be artificially) its other uses unrelated to this thread.

>Does the original problem exist in HTTPv2?  

I tried the following *with* my patch 2 days back, 

With Master and slave both being in 1.7.x 

ra_serf do *not* suffer from this problem(I committed text mod to one file) as 
it do not make the call to PROPFIND. It makes some POST requests to do the 
commit, Not sure how that one is immune to this issue.

Will delve in to it tomorrow.


>At a minimum, could the ra_dav providers not annotate the PROPFIND as
>"dont-proxy-this" without even touching the RA (and higher) APIs?

Rather we want 'proxy-this', anyway I could *not* find a easy way to figure out 
whether a given request PROPFIND is for read or write operation.

With regards
Kamesh Jayachandran

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


>Let me see if I've got this right.  There is a MKACTIVITY that goes to
>the master, then a PROPFIND that goes to the slave, and finally a
>CHECKOUT that goes to master.  The MKACTIVITY and CHECKOUT go to the
>master because they are obviously write operations, the PROPFIND goes
>to the slave because it cannot be distinguished from a read-only
>operation that doesn't need to access the master.  The PROPFIND
>returns out-of-date info that causes the CHECKOUT to fail.

Yes you got it correct.

>You mentioned an objection from Justin related to old clients, is this
>it:

>http://mail-archives.apache.org/mod_mbox/subversion-dev/200912.mbox/<5c902b9e0912220853j110d893ewe412259e48b7b...@mail.gmail.com>

Yes.

>Is that about old clients or is it more about things like serf that
>use multiple connections?

It can even happen to neon connections if the client exceeded the maximum 
requests allowed per connection.

Or some forced closure of the connection due to intermittent network issues on 
client or server end.

With regards
Kamesh Jayachandran

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

I (Julian Foad) wrote:
> On Wed, 2010-01-06 at 08:37 -0800, Justin Erenkrantz wrote:
> > On Wed, Jan 6, 2010 at 7:54 AM, Mark Phippard  wrote:
> > > Apparently, the PROPFIND is using the proxy at a time when it needs to
> > > be using the master.
> > 
> > The client has no knowledge of the master - the slave a completely
> > transparent proxy.  The only way to distinguish between a PROPFIND for
> > an update or a commit is to have the client give some indication about
> > what it's intending.
> 
> It doesn't sound like we're asking quite the right question. Rather than
> "Is this PROPFIND for an update or is it for a commit?", I think we need
> to be asking something like "Does this PROPFIND need to see a more
> recent revision than the latest cached revision? It does if this client
> has (ever) created or is currently creating a newer revision."
> 
> Another way to look at the whole issue is that the proxy should give the
> client a consistent view of what revision is "head". It doesn't matter
> if that revision number is a bit older than the master's head, but as
> soon as that client has reason to have any knowledge of a newer revision
> (e.g. by doing a commit), the proxy should from that moment onwards
> provide a view of that newer revision.
> 
> The question then is how to identify "this client" - if the best
> information we have is to recognize "this client connection" then we
> can't support multi-connection requests automatically (except by
> heuristics). In that case, the information the client needs to supply is
> some sort of unique client id, or alternatively its last known head
> revision number. I don't think any sort of "operation name" is
> sufficient, because, for example, "svn commit" followed immediately by
> "svn up" should be guaranteed to update the WC to at least the revision
> number of the commit, whereas with the proposed operation-name scheme I
> think it would update to the last cached revision which can be older
> than the commit.

When I say, "I think ...", I mean that I am just thinking through in my
head how I suppose this would work. I hope someone more knoledgeable or
who is prepared to test it will correct me if I'm wrong.

- Julian

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

On Wed, 2010-01-06 at 08:37 -0800, Justin Erenkrantz wrote:
> On Wed, Jan 6, 2010 at 7:54 AM, Mark Phippard  wrote:
> > Apparently, the PROPFIND is using the proxy at a time when it needs to
> > be using the master.
> 
> The client has no knowledge of the master - the slave a completely
> transparent proxy.  The only way to distinguish between a PROPFIND for
> an update or a commit is to have the client give some indication about
> what it's intending.

It doesn't sound like we're asking quite the right question. Rather than
"Is this PROPFIND for an update or is it for a commit?", I think we need
to be asking something like "Does this PROPFIND need to see a more
recent revision than the latest cached revision? It does if this client
has (ever) created or is currently creating a newer revision."

Another way to look at the whole issue is that the proxy should give the
client a consistent view of what revision is "head". It doesn't matter
if that revision number is a bit older than the master's head, but as
soon as that client has reason to have any knowledge of a newer revision
(e.g. by doing a commit), the proxy should from that moment onwards
provide a view of that newer revision.

The question then is how to identify "this client" - if the best
information we have is to recognize "this client connection" then we
can't support multi-connection requests automatically (except by
heuristics). In that case, the information the client needs to supply is
some sort of unique client id, or alternatively its last known head
revision number. I don't think any sort of "operation name" is
sufficient, because, for example, "svn commit" followed immediately by
"svn up" should be guaranteed to update the WC to at least the revision
number of the commit, whereas with the proposed operation-name scheme I
think it would update to the last cached revision which can be older
than the commit.

- Julian

>   By using an HTTP header, the slave can check
> header and then proxy that PROPFIND to the master.  (This will likely
> make all PROPFIND's on a commit hit the server, but, eh, so be it.)
> -- justin

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

2010-01-06 Thread Justin Erenkrantz

On Wed, Jan 6, 2010 at 7:54 AM, Mark Phippard  wrote:
> Apparently, the PROPFIND is using the proxy at a time when it needs to
> be using the master.

The client has no knowledge of the master - the slave a completely
transparent proxy.  The only way to distinguish between a PROPFIND for
an update or a commit is to have the client give some indication about
what it's intending.  By using an HTTP header, the slave can check
header and then proxy that PROPFIND to the master.  (This will likely
make all PROPFIND's on a commit hit the server, but, eh, so be it.)
-- justin

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

2010-01-06 Thread Mark Phippard

On Wed, Jan 6, 2010 at 10:48 AM, Julian Foad  wrote:

> 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.

My understanding is that these commits fail, so I do not think it is
about accessing the new revision at all.  I think it has something to
do with establishing the baseline revision for the commit and picking
an out of date revision from the proxy and then having the master
reject it.

It goes back to this I think:

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

Apparently, the PROPFIND is using the proxy at a time when it needs to
be using the master.

I think your general question stands, although this is only ever going
to be an issue when we are talking about some kind of write operation,
so it ought to be fairly easy to know where we have a problem.

-- 
Thanks

Mark Phippard
http://markphip.blogspot.com/

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

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:kam...@collab.net]
&g

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

2010-01-06 Thread C. Michael Pilato

Philip Martin wrote:
> Kamesh Jayachandran  writes:
> 
>> This patch is with respect to the original thread
>>
>> http://mail-archives.apache.org/mod_mbox/subversion-dev/201001.mbox/browser
> 
> This one I suppose:
> 
> http://mail-archives.apache.org/mod_mbox/subversion-dev/201001.mbox/<4b41f1bd.8090...@collab.net>
> 
> It includes:
> 
>"We can proxy this request to the Master but we *should not* do
> that if it is for read operation."

With all due respect, the proposed solution looks enormous compared to the
size of the problem.   Does the original problem exist in HTTPv2?  At a
minimum, could the ra_dav providers not annotate the PROPFIND as
"dont-proxy-this" without even touching the RA (and higher) APIs?

-- 
C. Michael Pilato 
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand

signature.asc
Description: OpenPGP digital signature

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

Kamesh Jayachandran  writes:

>> Would it be possible for the server to detect the slave error and
>> then try the master?
>
> In fact Master is the one that gives the vague error as snipped below.
>
> 
>
> "The specified baseline is not the latest baseline, so it may not be
> checked out."  
>
> But it would be too late for the Master to tell anything meaningful to
> Slave(if at all such mechanisms exist).

Let me see if I've got this right.  There is a MKACTIVITY that goes to
the master, then a PROPFIND that goes to the slave, and finally a
CHECKOUT that goes to master.  The MKACTIVITY and CHECKOUT go to the
master because they are obviously write operations, the PROPFIND goes
to the slave because it cannot be distinguished from a read-only
operation that doesn't need to access the master.  The PROPFIND
returns out-of-date info that causes the CHECKOUT to fail.

You mentioned an objection from Justin related to old clients, is this
it:

http://mail-archives.apache.org/mod_mbox/subversion-dev/200912.mbox/<5c902b9e0912220853j110d893ewe412259e48b7b...@mail.gmail.com>

Is that about old clients or is it more about things like serf that
use multiple connections?

-- 
Philip

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

On 01/06/2010 06:58 PM, Bert Huijben wrote:

-Original Message-
From: Kamesh Jayachandran [mailto:kam...@collab.net]
Sent: woensdag 6 januari 2010 14:00
To: dev@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'.

First We need this change as an easy means to tell the server what the 
client's intensions are.

We are not bothered about the value of SVN-ACTION except when it is 
'commit'.

If the user uses old client(<1.7) we have a fall back mechanism in place 
to detect.

This fall back mechanism *relies* on connection persistence which is not 
a reliable assumption as per Justin.

So we have this per request logging.

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?

You can put anything you like that identifies the new operation you come 
up with.

Suppose some gui svn app has a feature by name 'merge aware revision 
graph' a custom implementation can open the ra_session with the string 
'merge-aware-revision-graph' So that server admin can identify it if needed.

"checking-uuid-for-visualization-to-my-great-users"?

Yes, it can be any string.

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.

We need to rev this as we do not want to know 'micro operations like 
getting UUID' rather 'SOME custom command which makes use of this 
self-contained utility function'.

And it should never be forwarded to master servers as the uuid is supposed
to be constant per repository.

Client never *knows* about the master.

This implementation is just one Broad level identification tied to sub 
requests helping with big operation.

(BTW. the api is new in 1.7, so it needs no revving at all)

Yes it checked it seemed to be there since 2003.

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?

Server do not bother about at it, It just gets the clue only when the 
operation is 'commit'.

It *can* be useful for some server admin to keep track of how his server 
resources are utilised.

May be if he sees some operation by name 
'SERVER-RESOURCE-INTENSIVE-OPERATION' as its SVN-ACTION header he can 
deny the connection atleast temporarily till he equips his server to 
handle this intensive operation.

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?

No ra sessions do not even know the existence of slave. Only proxy knows 
that it is a proxy *not* anyone else.

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.

May be we can implement the svn_ra_redescribe or something to tell its 
new intensions.

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.

No need, clients can/should not know anything about Master.

With regards
Kamesh Jayachandran

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




Would it be possible for the server to detect the slave error and then
try the master?

   


In fact Master is the one that gives the vague error as snipped below.



"The specified baseline is not the latest baseline, so it may not be checked
out."


But it would be too late for the Master to tell anything meaningful to Slave(if 
at all such mechanisms exist).

With regards
Kamesh Jayachandran

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

Kamesh Jayachandran  writes:

> 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'.

That does sound like a better solution, but I suppose it depends on
how clients use connections.

>
> 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.

Would it be possible for the server to detect the slave error and then
try the master?

-- 
Philip

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


Julian,


Let me explain the strategy.

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.


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:kam...@collab.net]
Sent: woensdag 6 januari 2010 14:00
To: dev@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 soluti

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


On 01/06/2010 07:32 PM, Philip Martin wrote:

Kamesh Jayachandran  writes:

   

This patch is with respect to the original thread

http://mail-archives.apache.org/mod_mbox/subversion-dev/201001.mbox/browser
 

This one I suppose:

http://mail-archives.apache.org/mod_mbox/subversion-dev/201001.mbox/<4b41f1bd.8090...@collab.net>
   


Yes, thanks for correcting.


It includes:

"We can proxy this request to the Master but we *should not* do
 that if it is for read operation."

Does something go wrong if the read operation goes to the master or is
it just an efficiency thing?
   


It can go wrong in the following scenario,

If Slave is out-dated still checkouts from it should work without any 
confusion. If we always proxy 'PROPFIND' to Master we may get some new 
revision numbers operated against the Slave and hence loads of confusion.




   

Once this patch gets committed I can commit the mod_dav_svn change to
handle the original commit via outdated proxy issue.
 

Have we seen this other patch yet?
   


Not yet, Find it in the attachment. This fix expects the new header 
'SVN-ACTION', if it does not exist makes use of 'connection persistence' 
to find the commit activity and proxy selectively.



   

This Patch revs the following public APIs,

'svn_client_uuid_from_url', 'svn_client_open_ra_session' and 'svn_ra_open3'.

For ra_neon and ra_serf layers it sets the http client header
SVN-ACTION with the concerned svn command name.
 

The "SVN-ACTION" name is already used by the operational logging code,
I don't know if this matters.

   


May be in the *future* when the new client is everywhere we can change 
the operational logging code to make use of this header.


May be I can change this to something SVN-COMMAND if there are serious 
objections.





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.
 

This ties an RA session to a single operation name -- that's a new
restriction on RA sessions isn't it?  It may be the way our clients
work at present but clients don't have to work like that, a single
session could be used for multiple operations.  A client may not even
know what operations are going to be carried out until some time after
the session has been opened.
   


May be we can provide an RA API to change the command names prior to 
using the pre-existing ra_session for new command.



With regards
Kamesh Jayachandran
Index: subversion/mod_dav_svn/mirror.c
===
--- subversion/mod_dav_svn/mirror.c (revision 896432)
+++ subversion/mod_dav_svn/mirror.c (working copy)
@@ -30,7 +30,54 @@
 
 #include "dav_svn.h"
 
+#define USERDATA_KEY "mod_dav_svn_userdata_key"
 
+typedef struct
+{
+  int mk_activity_has_happened;
+} ctx_data;
+
+static apr_status_t
+initialize_ctx_data(ctx_data **data,
+request_rec *r,
+apr_pool_t *pool)
+{
+  ctx_data *ctx = apr_pcalloc(pool, sizeof(*ctx));
+
+  if (apr_pool_userdata_set(ctx, USERDATA_KEY, NULL, pool) != APR_SUCCESS)
+{
+  ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
+"mod_dav_svn: Error setting connection pool userdata");
+  return HTTP_INTERNAL_SERVER_ERROR;
+}
+  ctx->mk_activity_has_happened = 0;
+  *data = ctx;
+  return APR_SUCCESS;
+}
+
+static int 
+get_userdata(ctx_data **ctx,
+ request_rec *r)
+{
+  void *data;
+  apr_pool_t *pool = r->connection->pool;
+
+  if (apr_pool_userdata_get(&data, USERDATA_KEY, pool) != APR_SUCCESS)
+{
+  ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
+"mod_dav_svn: Error fetching connection userdata");
+  return HTTP_INTERNAL_SERVER_ERROR;
+}
+
+  if (data)
+*ctx = data;
+  else
+initialize_ctx_data(ctx, r, pool);
+
+  return OK;
+}
+
+
 /* Tweak the request record R, and add the necessary filters, so that
the request is ready to be proxied away.  MASTER_URI is the URI
specified in the SVNMasterURI Apache configuration value.
@@ -57,6 +104,7 @@
 int dav_svn__proxy_merge_fixup(request_rec *r)
 {
 const char *root_dir, *master_uri, *special_uri;
+FILE *fp;
 
 root_dir = dav_svn__get_root_dir(r);
 master_uri = dav_svn__get_master_uri(r);
@@ -64,6 +112,14 @@
 
 if (root_dir && master_uri) {
 const char *seg;
+const char *action;
+int is_commit = 0;
+if (r->method_number == M_MKACTIVITY) {
+ctx_data *ctx;
+int ret = get_userdata(&ctx, r);
+if (ret == OK)
+ctx->mk_activity_has_happened = 1;
+}
 
 /* We know we can always safely handle these. */
 if (r->method_number == M_REPORT ||
@@ -71,6 +127,18 @@
 return OK;
 }
 
+action = apr_table_get(r->headers_in, "SVN-ACTION");
+if (action) {
+if (strcmp(action, "commit") == 0)
+

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

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
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
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.)

- Julian


Bert Huijben wrote:
> 
> > -Original Message-
> > From: Kamesh Jayachandran [mailto:kam...@collab.net]
> > Sent: woensdag 6 januari 2010 14:00
> > To: dev@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
>

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

Kamesh Jayachandran  writes:

> This patch is with respect to the original thread
>
> http://mail-archives.apache.org/mod_mbox/subversion-dev/201001.mbox/browser

This one I suppose:

http://mail-archives.apache.org/mod_mbox/subversion-dev/201001.mbox/<4b41f1bd.8090...@collab.net>

It includes:

   "We can proxy this request to the Master but we *should not* do
that if it is for read operation."

Does something go wrong if the read operation goes to the master or is
it just an efficiency thing?

> Once this patch gets committed I can commit the mod_dav_svn change to
> handle the original commit via outdated proxy issue.

Have we seen this other patch yet?

> This Patch revs the following public APIs,
>
> 'svn_client_uuid_from_url', 'svn_client_open_ra_session' and 'svn_ra_open3'.
>
> For ra_neon and ra_serf layers it sets the http client header
> SVN-ACTION with the concerned svn command name.

The "SVN-ACTION" name is already used by the operational logging code,
I don't know if this matters.

> 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.

This ties an RA session to a single operation name -- that's a new
restriction on RA sessions isn't it?  It may be the way our clients
work at present but clients don't have to work like that, a single
session could be used for multiple operations.  A client may not even
know what operations are going to be carried out until some time after
the session has been opened.

-- 
Philip

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

2010-01-06 Thread Bert Huijben



> -Original Message-
> From: Bert Huijben [mailto:b...@qqmail.nl]
> Sent: woensdag 6 januari 2010 14:28
> To: 'Kamesh Jayachandran'; dev@subversion.apache.org
> Subject: RE: [PATCH] Make svn clients indicate their operation name to
> backend(right now only to DAV)
> 
> 
> 
> > -Original Message-
> > From: Kamesh Jayachandran [mailto:kam...@collab.net]
> > Sent: woensdag 6 januari 2010 14:00
> > To: dev@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)

Forget this last sentence, it was not new. It was there since before 1.0

Bert

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

2010-01-06 Thread Bert Huijben



> -Original Message-
> From: Kamesh Jayachandran [mailto:kam...@collab.net]
> Sent: woensdag 6 januari 2010 14:00
> To: dev@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

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