Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm
Precedence: bulk
MIME-Version: 1.0
In-Reply-To: <073301d1231c$7560f540$6022dfc0$@qqmail.nl>
References: <20151119180312.8FB3E3A00E5@svn01-us-west.apache.org>
 <072601d1230d$be972ae0$3bc580a0$@qqmail.nl>
 <CABw-3YdW29U+ATUfdwxJfca32vhq2+oiwouPbDMgvqFAoCmf_g@mail.gmail.com>
 <073201d1231b$4fc08040$ef4180c0$@qqmail.nl>
 <073301d1231c$7560f540$6022dfc0$@qqmail.nl>
From: Ivan Zhakov <ivan@visualsvn.com>
Date: Fri, 20 Nov 2015 09:54:29 +0300
Message-ID: 
 <CABw-3Yfv0=KzC_RJnDGGfREx3ex0nb8aJaSavnLdALJ5PJ60oQ@mail.gmail.com>
Subject: Re: svn commit: r1715228 -
 /subversion/trunk/subversion/libsvn_ra_serf/util.c
To: Bert Huijben <bert@qqmail.nl>
Cc: dev@subversion.apache.org
Content-Type: text/plain; charset=UTF-8

On 20 November 2015 at 01:48, Bert Huijben <bert@qqmail.nl> wrote:
>> -----Original Message-----
>> From: Bert Huijben [mailto:bert@qqmail.nl]
>> Sent: donderdag 19 november 2015 23:41
>> To: 'Ivan Zhakov' <ivan@visualsvn.com>
>> Cc: dev@subversion.apache.org
>> Subject: RE: svn commit: r1715228 -
>> /subversion/trunk/subversion/libsvn_ra_serf/util.c
>>
>>
>>
>> > -----Original Message-----
>> > From: Ivan Zhakov [mailto:ivan@visualsvn.com]
>> > Sent: donderdag 19 november 2015 23:12
>> > To: Bert Huijben <bert@qqmail.nl>
>> > Cc: dev@subversion.apache.org
>> > Subject: Re: svn commit: r1715228 -
>> > /subversion/trunk/subversion/libsvn_ra_serf/util.c
>> >
>> > On 20 November 2015 at 00:03, Bert Huijben <bert@qqmail.nl> wrote:
>> > >> -----Original Message-----
>> > >> From: rhuijben@apache.org [mailto:rhuijben@apache.org]
>> > >> Sent: donderdag 19 november 2015 19:03
>> > >> To: commits@subversion.apache.org
>> > >> Subject: svn commit: r1715228 -
>> > >> /subversion/trunk/subversion/libsvn_ra_serf/util.c
>> > >>
>> > >> Author: rhuijben
>> > >> Date: Thu Nov 19 18:03:11 2015
>> > >> New Revision: 1715228
>> > >>
>> > >> URL: http://svn.apache.org/viewvc?rev=1715228&view=rev
>> > >> Log:
>> > >> Add a tiny bit of code to allow testing with Apache Serf's http/2 support.
>> > >>
>> > >> I committed this patch to celebrate that I got through basic_tests.py
>> > >> using the current http/2 support.
>> > >>
>> > >> * subversion/libsvn_ra_serf/util.c
>> > >>   (conn_negotiate_protocol): New function.
>> > >>   (conn_setup): Register usage of conn_negotiate_protocol when
>> > >>     a very recent (read: trunk) serf + SVN__SERF_TEST_HTTP2 are used.
>> > >
>> > > Currently most tests pass for me over http2 when running with some
>> minor
>> > tweaks. I still get some aborted connections that aren't retried as cleanly as
>> > with http 1.1. Not sure what causes these yet; but this could also be an
>> httpd
>> > issues.
>> > >
>> > > The only ra function that really causes problems is the implementation of
>> > the replay
>> > > range api. This 100% expects that results will be received in the same
>> order
>> > in which
>> > > they are sent. This is typically the correct way in http 1.1 using serf, but
>> > even there it
>> > > is sometimes possible to get results out of order. (Authentication can re-
>> > queue
>> > > requests... and sometimes authorization is necessary even when earlier
>> > requests
>> > > succeeded, depending on the auth scheme).
>> > >
>> > Can we use WINDOW_UPDATE frame to control flow of replay-revision
>> > response? In this case we can use spillbuf for buffering responses to
>> > avoid latency performance regressions. What do you think?
>>
>> In theory we could... we could set the initial window to 0 and then send a
>> window update later. Assuming that httpd does that efficiently, etc. etc.
>> (Some of these assumptions are most likely not true)
>>
>> But I doubt if it would be more efficient than just sending the request when
>> we need it... or perhaps asking the properties a revision earlier. (We can also
>> send the request, but just leave out the end of stream marker... Sending EOS
>> will then handle the request)
>
> The proper 2.0 way would be to make the streams completely dependent on each other via priority handling.
>
> But using this priority information is optional on the server side. And I doubt that
> we can trust this to always do the right thing even if httpd would implement this
> correctly today.
>
Yes, stream priorities is another option, but it's not reliable. We
still have to manage spillbuf as fallback etc.

I agree that current code could fail in some edge cases, like
re-authentication for some reason.

I think simple solution for all these kind of problems would be just
add new replay-range REPORT that will stream all revision range in one
REPORT. We already do this for blame in file-revs REPORT, so I don't
see problems here. For older servers we can just disable pipelining in
this case. What do you think?

-- 
Ivan Zhakov