Re: svn commit: r1501038 - /subversion/branches/1.8.x/STATUS
On Jul 9, 2013 12:52 PM, "Ben Reser" wrote: > > On Tue, Jul 9, 2013 at 12:55 AM, Ben Reser wrote: > > Have we considered just turning off chunked requests entirely until > > Serf has a fix to automatically detect and handle servers that don't > > support chunked requests? I've seen a lot of discussion about the > > cost of doing an extra round trip to detect if we can use chunked > > encoding, but I haven't seen anyone comparing the performance of the > > client with and without chunked requests. > > Okay talked with Ivan and Lieven on IRC and they explained why this > isn't an option. Essentially Serf isn't as efficient as Neon was with > doing CL requests. We produce a temp file with the body, then send > the data to serf which would produce another temp file and calculate > the size. Which makes this suggestion a bad idea. For those on the list who didn't hear why... Neon is synchronous, so the app (svn) writes into Neon, which writes the data into the socket. Serf is asynchronous, and can manage many connections at once. Effectively, it uses a callback when a connection is available for writing. The callback provides data, and it gets written to the socket. In our case, the app writes to a temp file, and the callback reads from that file. We can optimize some requests by using spillbuf. Data will stay in memory, but spill to a file if it gets too large. Of course, we don't want 100,000 spillbufs at 1M each. But there are ways to figure it out, or only requests that won't be too numerous. Another possibility is for the callback to generate the request on demand, rather than having the app write it synchronously. This is my preferred approach, but much of svn's APIs are push-based, making this difficult. Cheers, -g
Re: svn commit: r1501038 - /subversion/branches/1.8.x/STATUS
On Tue, Jul 9, 2013 at 12:55 AM, Ben Reser wrote: > Have we considered just turning off chunked requests entirely until > Serf has a fix to automatically detect and handle servers that don't > support chunked requests? I've seen a lot of discussion about the > cost of doing an extra round trip to detect if we can use chunked > encoding, but I haven't seen anyone comparing the performance of the > client with and without chunked requests. Okay talked with Ivan and Lieven on IRC and they explained why this isn't an option. Essentially Serf isn't as efficient as Neon was with doing CL requests. We produce a temp file with the body, then send the data to serf which would produce another temp file and calculate the size. Which makes this suggestion a bad idea.
Re: svn commit: r1501038 - /subversion/branches/1.8.x/STATUS
Lieven Govaerts writes: > First of all, because that's not guaranteed to be or stay that way. > Take for instance svnsync. It currently sends two non-pipelined > OPTIONS requests (to my surprise) from: > 1. svn_ra_serf__exchange_capabilities > 2. svn_ra_serf__v2_get_youngest_revnum > > I consider the request sent by svn_ra_serf__v2_get_youngest_revnum in > this case a bug. The second OPTIONS request and response is identical > to the first, the only reason why it's sent is because ra_serf doesn't > cache the latest revision number from the response to the first > request. > > So suppose I fix that bug, your assumption isn't valid anymore for svnsync. It's possible that some operations will fail, but if they fail they will do so in exactly the same way they currently fail right now. The user handles it in exactly the same way: modify the config to enable the extra OPTIONS probe. > Second, IMO the bottleneck is not in having to send the extra second > request, but the fact that we have to wait for the response to any > second request before we can start pipelining the 3rd,4d... request. > It's better to send two small requests to find out the capabilities of > the whole chain asap, instead of having to retry a potentially > expensive update REPORT (takes time to create, to spool to disk, to > re-read and stream over the network). Not all REPORTs are expensive. Your example, syncsync, sends very simple REPORTs. It's much more convenient for the user if the client just works. We can still notify to suggest that the user sets the config option to enable the OPTIONS probe. -- Philip Martin | Subversion Committer WANdisco | Non-Stop Data www.wandisco.com
Re: svn commit: r1501038 - /subversion/branches/1.8.x/STATUS
Hi Philip, On Tue, Jul 9, 2013 at 12:01 PM, Philip Martin wrote: > Lieven Govaerts writes: > >> Note, again: Serf will not have a feature to automatically detect that >> servers don't support chunked requests. Such a check is implemented in >> ra_serf with the extra OPTIONS request, there's no other way to do >> this without the extra roundtrip. > > There is another way: rely on the fact that all client operations from > the client start with non-pipelined requests. During this phase ra_serf > recognises the 411 for the first chunked request, switches to C-L mode > and retries the request. There is still an extra request, but only when > the proxy is present. My patch to implement this allows 90% of the > regression tests to PASS, if I could get the ra_serf code to retry the > update REPORT request I think just about all the tests would PASS. I wouldn't make that assumption. First of all, because that's not guaranteed to be or stay that way. Take for instance svnsync. It currently sends two non-pipelined OPTIONS requests (to my surprise) from: 1. svn_ra_serf__exchange_capabilities 2. svn_ra_serf__v2_get_youngest_revnum I consider the request sent by svn_ra_serf__v2_get_youngest_revnum in this case a bug. The second OPTIONS request and response is identical to the first, the only reason why it's sent is because ra_serf doesn't cache the latest revision number from the response to the first request. So suppose I fix that bug, your assumption isn't valid anymore for svnsync. Second, IMO the bottleneck is not in having to send the extra second request, but the fact that we have to wait for the response to any second request before we can start pipelining the 3rd,4d... request. It's better to send two small requests to find out the capabilities of the whole chain asap, instead of having to retry a potentially expensive update REPORT (takes time to create, to spool to disk, to re-read and stream over the network). > -- > Philip Martin | Subversion Committer > WANdisco | Non-Stop Data > www.wandisco.com Lieven
Re: svn commit: r1501038 - /subversion/branches/1.8.x/STATUS
Lieven Govaerts writes: > Note, again: Serf will not have a feature to automatically detect that > servers don't support chunked requests. Such a check is implemented in > ra_serf with the extra OPTIONS request, there's no other way to do > this without the extra roundtrip. There is another way: rely on the fact that all client operations from the client start with non-pipelined requests. During this phase ra_serf recognises the 411 for the first chunked request, switches to C-L mode and retries the request. There is still an extra request, but only when the proxy is present. My patch to implement this allows 90% of the regression tests to PASS, if I could get the ra_serf code to retry the update REPORT request I think just about all the tests would PASS. -- Philip Martin | Subversion Committer WANdisco | Non-Stop Data www.wandisco.com
Re: svn commit: r1501038 - /subversion/branches/1.8.x/STATUS
On Tue, Jul 9, 2013 at 9:55 AM, Ben Reser wrote: [..] > Have we considered just turning off chunked requests entirely until > Serf has a fix to automatically detect and handle servers that don't > support chunked requests? Note, again: Serf will not have a feature to automatically detect that servers don't support chunked requests. Such a check is implemented in ra_serf with the extra OPTIONS request, there's no other way to do this without the extra roundtrip. What Serf 1.4 will have is a way to calculate the length of buckets without overhead, so that ra_serf can then add the Content-Length header to each request. > I've seen a lot of discussion about the > cost of doing an extra round trip to detect if we can use chunked > encoding, but I haven't seen anyone comparing the performance of the > client with and without chunked requests. The cost of setting the Content-Length header on each request at this time (serf 1.2.1/serf 1.3), is that each request will be read completely in memory (a second copy of the request in memory) or spooled to disk for requests larger than 32MB. See setup_serf_req in ra_serf/util.c. If a request body was already persisted to disk first (e.g. commit), it will also follow the same procedure. I haven't measured the effect of all this, but you see that there's an overhead involved. Serf 1.4 will solve this because then ra_serf can ask the request body bucket for its complete length without needing to read the whole bucket completely first. Lieven
Re: svn commit: r1501038 - /subversion/branches/1.8.x/STATUS
On Mon, Jul 8, 2013 at 9:52 PM, Greg Stein wrote: > If you're going to vote this way, then I think you need to say what would > solve the issue for you. We probably don't want to play "bring me another > rock". I'm pretty sure that Daniel would accept my suggestion in the other thread of [[[ I'd suggest http-detect-chunking with a description of "Detect if the server supports chunked requests. This may be necessary for use when a proxy does not support chunked requests. By default this is off because mod_dav_svn always supports chunked requests and detection of this hurts performance." ]]] > Note: I see the term "busted" is from svn's point of view, NOT the protocol > definition, and since this is *our* config, then it is entirely reasonable > to use that point of view. That's not what you've been arguing at all until now. You've been arguing that chunked requests is in the HTTP/1.1 spec and they're required to implement it. But even this point of view argument isn't valid. Because the proxies are actually busted from Serf's point of view because Subversion prior to Serf had no problem with these servers. We've made the decision to switch to Serf because it has some benefits, one of which is chunked requests. Telling users that their proxy is busted from our point of view when it worked just fine with 1.7 doesn't go over very well with users. As Kevin rightly pointed out, users really don't care that you think every HTTP/1.1 server should be implementing chunked requests. > Consider: what if the server responded 501 to every MERGE request? That > meets the protocol definition, but it is absolutely busted from svn's point > of view. That's not a reasonable comparison at all. There is absolutely no way to resolve that on our end. Yes we expect that the DAV/DeltaV methods work. There is a world of difference between requiring some functionality to work that we've never worked without before and requiring an optional feature of HTTP/1.1 work when we haven't required it up until now and the HTTP/1.1 standard is essentially telling you that you really should avoid using the feature unless you have to use it. We have a working patch that detects this issue by sending an extra request and then turning off chunked requests so we don't have to use this. When we add new functionality to our servers and clients we negotiate what we can use. A 1.8.x client continues to work against a 1.7.x server even though some of the features on the server end a 1.8.x client might want to use. This should be absolutely no different. Have we considered just turning off chunked requests entirely until Serf has a fix to automatically detect and handle servers that don't support chunked requests? I've seen a lot of discussion about the cost of doing an extra round trip to detect if we can use chunked encoding, but I haven't seen anyone comparing the performance of the client with and without chunked requests.
Re: svn commit: r1501038 - /subversion/branches/1.8.x/STATUS
On Tue, Jul 09, 2013 at 12:52:34AM -0400, Greg Stein wrote: > If you're going to vote this way, then I think you need to say what would > solve the issue for you. "http-probe-chunking" > We probably don't want to play "bring me another rock". > > Note: I see the term "busted" is from svn's point of view, NOT the protocol > definition, and since this is *our* config, then it is entirely reasonable > to use that point of view. > > Consider: what if the server responded 501 to every MERGE request? That > meets the protocol definition, but it is absolutely busted from svn's point > of view. > > Our config options are for svn. Not for the protocol. > Our config options should describe what they do. "busted-proxy" does not describe the option, it describes the use-case that gave rise to it and our judgement thereof. If we face a server that replaces half of the bytes in every response body with NULs, it's irrelevant whether that server is busted, smells, or runs Framedglasspanes; the option we might add to work around that should be called "ask-server-to-write-every-byte-twice". Daniel > Cheers, > -g > > > On Mon, Jul 8, 2013 at 9:18 PM, wrote: > > > Author: danielsh > > Date: Tue Jul 9 01:18:43 2013 > > New Revision: 1501038 > > > > URL: http://svn.apache.org/r1501038 > > Log: > > * STATUS: Veto 'busted-proxy'. > > > > Modified: > > subversion/branches/1.8.x/STATUS > > > > Modified: subversion/branches/1.8.x/STATUS > > URL: > > http://svn.apache.org/viewvc/subversion/branches/1.8.x/STATUS?rev=1501038&r1=1501037&r2=1501038&view=diff > > > > == > > --- subversion/branches/1.8.x/STATUS (original) > > +++ subversion/branches/1.8.x/STATUS Tue Jul 9 01:18:43 2013 > > @@ -55,6 +55,61 @@ Candidate changes: > > Veto-blocked changes: > > = > > > > + * r1489117, r1496470, r1497975, r1497980, r1498012, r1499423, r1499595 > > + Support for HTTP/1.1 reverse proxies that require Content-Length > > headers > > + and deny chunked requests. > > + Branch: ^/subversion/branches/1.8.x-busted-proxy > > + Justification: > > + Older nginx servers cannot handle chunked requests. This patch will > > + keep HTTP/1.1 features in the connection, but will always use C-L > > + headers for the length (at a variant performance cost). This > > increases > > + svn's compatibility in various environments. Most installations will > > + never see this; the runtime test is only enabled with a config > > settting. > > + Notes: > > + r1489117: this is actually unrelated to the proxy work, but it > > removes > > + some redundant minimum-serf-version checking and allows the > > + actual work to cleanly merge. > > + r1496470: add/respect using_chunked_requests flag to session state > > + r1497975: rename config variable, tweak session variables > > + r1497980: add runtime/dynamic checking of proxy support > > + r1498012: remove improper check of client-side proxy; the real > > problem > > + is within the server-side reverse proxy > > + r1499423: fix 411 handling > > + r1499595: don't probe when a redirect occurs > > + > > + Local to branch: > > + r1499225: remove SVN_CONFIG_OPTION_BUSTED_PROXY from the public API, > > + since we cannot add a symbol in a patch release. > > + Votes: > > + +1: gstein, philip, lgo > > + -0: breser (option name will be regretted since RFC is likely > > + being updated to allow HTTP/1.1 without chunked) > > + -1: danielsh (option name is wrong. Those proxies follow the spec, > > so they > > + are not busted, period.) > > + > > + * r1500837 > > + Follow up to the r1489117 group, modifies the 411 Content length > > required > > + error message to point the user to the busted-proxy option. > > + Justification: > > + The original error message was meaningless, this patch at least > > guides > > + the user in the right direction. > > + Notes: > > + Depends on the r1489117 group (logically, not merge-wise). > > + Votes: > > + +1: lgo, rhuijben, danielsh, breser > > + -1: (not vetoed, but depends on the r1489117 group which is) > > + > > + * r1500857 > > + A further clarification after r1500837 > > + Justification: > > + Save users a tour to $SEARCH_ENGINE. > > + Depends: r1500837 > > + Votes: > > + +1: danielsh, lgo, breser > > + +1: rhuijben (but also +1 on not applying it if that would be the > > + consensus) > > + -1: (not vetoed, but depends on the r1489117 group which is) > > + > > Approved changes: > > = > > > > @@ -140,57 +195,6 @@ Approved changes: > > Votes: > > +1: stsp, danielsh, rhuijben, breser > > > > - * r1489117, r1496470, r1497975, r1497980, r1498012, r1499423, r1499595 > > - Support for HTTP/1.1 reverse proxies that require Content
Re: svn commit: r1501038 - /subversion/branches/1.8.x/STATUS
If you're going to vote this way, then I think you need to say what would solve the issue for you. We probably don't want to play "bring me another rock". Note: I see the term "busted" is from svn's point of view, NOT the protocol definition, and since this is *our* config, then it is entirely reasonable to use that point of view. Consider: what if the server responded 501 to every MERGE request? That meets the protocol definition, but it is absolutely busted from svn's point of view. Our config options are for svn. Not for the protocol. Cheers, -g On Mon, Jul 8, 2013 at 9:18 PM, wrote: > Author: danielsh > Date: Tue Jul 9 01:18:43 2013 > New Revision: 1501038 > > URL: http://svn.apache.org/r1501038 > Log: > * STATUS: Veto 'busted-proxy'. > > Modified: > subversion/branches/1.8.x/STATUS > > Modified: subversion/branches/1.8.x/STATUS > URL: > http://svn.apache.org/viewvc/subversion/branches/1.8.x/STATUS?rev=1501038&r1=1501037&r2=1501038&view=diff > > == > --- subversion/branches/1.8.x/STATUS (original) > +++ subversion/branches/1.8.x/STATUS Tue Jul 9 01:18:43 2013 > @@ -55,6 +55,61 @@ Candidate changes: > Veto-blocked changes: > = > > + * r1489117, r1496470, r1497975, r1497980, r1498012, r1499423, r1499595 > + Support for HTTP/1.1 reverse proxies that require Content-Length > headers > + and deny chunked requests. > + Branch: ^/subversion/branches/1.8.x-busted-proxy > + Justification: > + Older nginx servers cannot handle chunked requests. This patch will > + keep HTTP/1.1 features in the connection, but will always use C-L > + headers for the length (at a variant performance cost). This > increases > + svn's compatibility in various environments. Most installations will > + never see this; the runtime test is only enabled with a config > settting. > + Notes: > + r1489117: this is actually unrelated to the proxy work, but it > removes > + some redundant minimum-serf-version checking and allows the > + actual work to cleanly merge. > + r1496470: add/respect using_chunked_requests flag to session state > + r1497975: rename config variable, tweak session variables > + r1497980: add runtime/dynamic checking of proxy support > + r1498012: remove improper check of client-side proxy; the real > problem > + is within the server-side reverse proxy > + r1499423: fix 411 handling > + r1499595: don't probe when a redirect occurs > + > + Local to branch: > + r1499225: remove SVN_CONFIG_OPTION_BUSTED_PROXY from the public API, > + since we cannot add a symbol in a patch release. > + Votes: > + +1: gstein, philip, lgo > + -0: breser (option name will be regretted since RFC is likely > + being updated to allow HTTP/1.1 without chunked) > + -1: danielsh (option name is wrong. Those proxies follow the spec, > so they > + are not busted, period.) > + > + * r1500837 > + Follow up to the r1489117 group, modifies the 411 Content length > required > + error message to point the user to the busted-proxy option. > + Justification: > + The original error message was meaningless, this patch at least > guides > + the user in the right direction. > + Notes: > + Depends on the r1489117 group (logically, not merge-wise). > + Votes: > + +1: lgo, rhuijben, danielsh, breser > + -1: (not vetoed, but depends on the r1489117 group which is) > + > + * r1500857 > + A further clarification after r1500837 > + Justification: > + Save users a tour to $SEARCH_ENGINE. > + Depends: r1500837 > + Votes: > + +1: danielsh, lgo, breser > + +1: rhuijben (but also +1 on not applying it if that would be the > + consensus) > + -1: (not vetoed, but depends on the r1489117 group which is) > + > Approved changes: > = > > @@ -140,57 +195,6 @@ Approved changes: > Votes: > +1: stsp, danielsh, rhuijben, breser > > - * r1489117, r1496470, r1497975, r1497980, r1498012, r1499423, r1499595 > - Support for HTTP/1.1 reverse proxies that require Content-Length > headers > - and deny chunked requests. > - Branch: ^/subversion/branches/1.8.x-busted-proxy > - Justification: > - Older nginx servers cannot handle chunked requests. This patch will > - keep HTTP/1.1 features in the connection, but will always use C-L > - headers for the length (at a variant performance cost). This > increases > - svn's compatibility in various environments. Most installations will > - never see this; the runtime test is only enabled with a config > settting. > - Notes: > - r1489117: this is actually unrelated to the proxy work, but it > removes > - some redundant minimum-serf-version checking and allows the > - actual work to cleanly merge. > - r1496470: add/