You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Ivan Zhakov <iv...@visualsvn.com> on 2015/09/17 17:41:41 UTC
Merge ra-reuse-session branch to trunk or not?
Hi,
I think now is good moment to discuss whether we should merge
ra-reuse-session [1] branch to trunk or not: it's better to merge such
branch in the beginning of release cycle, to have more time to test
and dogfood.
The idea of the branch is pretty simple: add pool of RA sessions in
svn_client_ctx_t and use it when we're going to open new RA session in
libsvn_client. RA sessions are returned back to RA pool *only* by
explicit svn_client__ra_session_release() call. RA sessions *will not
be reused* if they are destroyed by APR pool cleanup.
Goals of the branch were:
1. Avoid opening many TCP connections and improve merge performance of WAN
2. Avoid opening many TCP connections and improve performance in some
non-typical cases like working copy with many svn:externals (issue
3763 [2]) or subtree svn:mergeinfo.
3. Avoid passing RA session around and writing custom code with
reparent RA session to interesting and back in libsvn_client.
4. Improve performance of GUI clients that perform several
svn_client_* operation using the same svn_client_ctx_t.
That branch is complete and ready for merging, but I'm still not sure
whether we should merge it or not.
Pros:
- It reduce number of TCP connection to something like one or two,
instead of 10-20 in some cases.
- It should theoretically improve performance oven WAN
- It allows us write cleaner since we don't manage RA sessions. See
patch in [4] as an example how code could change.
- Fixes issue 3763
Cons:
- In makes behavior less stable. RA session pool doesn't reuse
sessions that was unused for some time to avoid timeout issues
- There is the chance that we will try to reuse 'broken' RA session
due the bug and operation will fail
[1] https://svn.apache.org/repos/asf/subversion/branches/reuse-ra-session
[2] http://subversion.tigris.org/issues/show_bug.cgi?id=3763
[3] http://svn.haxx.se/dev/archive-2013-06/0164.shtml
[4] http://svn.haxx.se/dev/archive-2015-02/0054.shtml
--
Ivan Zhakov
Re: Merge ra-reuse-session branch to trunk or not?
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 19 September 2015 at 11:58, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 18 September 2015 at 10:43, Markus Schaber <m....@codesys.com> wrote:
>> Hi,
>>
>> Von: Ivan Zhakov [mailto:ivan@visualsvn.com]
>>> On 17 September 2015 at 21:53, Philip Martin <ph...@wandisco.com>
>>> wrote:
>>> > Ivan Zhakov <iv...@visualsvn.com> writes:
>>> >
>>> >> I think now is good moment to discuss whether we should merge
>>> >> ra-reuse-session [1] branch to trunk or not: it's better to merge
>>> >> such branch in the beginning of release cycle, to have more time to
>>> >> test and dogfood.
>>> >
>>> > +1 to merge.
>>> >
>>> >> Cons:
>>> >> - In makes behavior less stable. RA session pool doesn't reuse
>>> >> sessions that was unused for some time to avoid timeout issues
>>> >> - There is the chance that we will try to reuse 'broken' RA session
>>> >> due the bug and operation will fail
>>> >
>>> > Do you have a plan to fix this?
>>> I don't have specific to fix bug that didn't happen. But if we got one we
>>> have two directions:
>>> - Do not release RA session back to pool in specific case where we get it
>>> broken
>>> - Make RA session more resilent to errors. There is no reason why ra_svn
>>> cannot reconnect after TCP connection times out or something.
>>>
>>> > Detect the error from a broken RA
>>> > session and create another? Track the time when the session was last
>>> > used? Something else?
>>> >
>>> Current implementation tracks last time when session was used and do not
>>> reuse RA sessions that was inactive for 5 minutes.
>>
>> I created some of my own session reuse logic for the SharpSVN "SvnRemoteSession"
>> which is a wrapper around the RA sessions. For "connectionless" sessions like http(s),
>> I just reuse them, while for "connection" sessions (svn:// and especially svn+ssh://), is
>> end a small "ping" in form of an "stat" request to validate that the session is still
>> active. This gives me a high reliability, and a huge speedup over creating a new
>> session (especially for ssh connections), but still have the guarantee that I won't
>> hand out sessions which are totally broken.
>>
>> Maybe some similar scheme can be used here, possibly with a small time period
>> after last successful usage where the revalidation may be skipped.
>>
> Interesting proposal, but I think should not be used for validating
I meant "'stat' should not be used".
> since it may generate false authorization log records. I also think we
> should skip revalidation if session was inactive for short period:
> otherwise we end up with optimizing obtaining/releasing sessions to
> save revalidation round-trip.
>
> The best option would be incorporate all this logic to RA session
> itself: RA session knows all internal details and may issue OPTIONS
> request for example to validate connection state and open new
> connection on error. But I think all of this can be implemented in
> trunk.
>
--
Ivan Zhakov
Re: Merge ra-reuse-session branch to trunk or not?
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 18 September 2015 at 10:43, Markus Schaber <m....@codesys.com> wrote:
> Hi,
>
> Von: Ivan Zhakov [mailto:ivan@visualsvn.com]
>> On 17 September 2015 at 21:53, Philip Martin <ph...@wandisco.com>
>> wrote:
>> > Ivan Zhakov <iv...@visualsvn.com> writes:
>> >
>> >> I think now is good moment to discuss whether we should merge
>> >> ra-reuse-session [1] branch to trunk or not: it's better to merge
>> >> such branch in the beginning of release cycle, to have more time to
>> >> test and dogfood.
>> >
>> > +1 to merge.
>> >
>> >> Cons:
>> >> - In makes behavior less stable. RA session pool doesn't reuse
>> >> sessions that was unused for some time to avoid timeout issues
>> >> - There is the chance that we will try to reuse 'broken' RA session
>> >> due the bug and operation will fail
>> >
>> > Do you have a plan to fix this?
>> I don't have specific to fix bug that didn't happen. But if we got one we
>> have two directions:
>> - Do not release RA session back to pool in specific case where we get it
>> broken
>> - Make RA session more resilent to errors. There is no reason why ra_svn
>> cannot reconnect after TCP connection times out or something.
>>
>> > Detect the error from a broken RA
>> > session and create another? Track the time when the session was last
>> > used? Something else?
>> >
>> Current implementation tracks last time when session was used and do not
>> reuse RA sessions that was inactive for 5 minutes.
>
> I created some of my own session reuse logic for the SharpSVN "SvnRemoteSession"
> which is a wrapper around the RA sessions. For "connectionless" sessions like http(s),
> I just reuse them, while for "connection" sessions (svn:// and especially svn+ssh://), is
> end a small "ping" in form of an "stat" request to validate that the session is still
> active. This gives me a high reliability, and a huge speedup over creating a new
> session (especially for ssh connections), but still have the guarantee that I won't
> hand out sessions which are totally broken.
>
> Maybe some similar scheme can be used here, possibly with a small time period
> after last successful usage where the revalidation may be skipped.
>
Interesting proposal, but I think should not be used for validating
since it may generate false authorization log records. I also think we
should skip revalidation if session was inactive for short period:
otherwise we end up with optimizing obtaining/releasing sessions to
save revalidation round-trip.
The best option would be incorporate all this logic to RA session
itself: RA session knows all internal details and may issue OPTIONS
request for example to validate connection state and open new
connection on error. But I think all of this can be implemented in
trunk.
--
Ivan Zhakov
AW: Merge ra-reuse-session branch to trunk or not?
Posted by Markus Schaber <m....@codesys.com>.
Hi,
Von: Ivan Zhakov [mailto:ivan@visualsvn.com]
> On 17 September 2015 at 21:53, Philip Martin <ph...@wandisco.com>
> wrote:
> > Ivan Zhakov <iv...@visualsvn.com> writes:
> >
> >> I think now is good moment to discuss whether we should merge
> >> ra-reuse-session [1] branch to trunk or not: it's better to merge
> >> such branch in the beginning of release cycle, to have more time to
> >> test and dogfood.
> >
> > +1 to merge.
> >
> >> Cons:
> >> - In makes behavior less stable. RA session pool doesn't reuse
> >> sessions that was unused for some time to avoid timeout issues
> >> - There is the chance that we will try to reuse 'broken' RA session
> >> due the bug and operation will fail
> >
> > Do you have a plan to fix this?
> I don't have specific to fix bug that didn't happen. But if we got one we
> have two directions:
> - Do not release RA session back to pool in specific case where we get it
> broken
> - Make RA session more resilent to errors. There is no reason why ra_svn
> cannot reconnect after TCP connection times out or something.
>
> > Detect the error from a broken RA
> > session and create another? Track the time when the session was last
> > used? Something else?
> >
> Current implementation tracks last time when session was used and do not
> reuse RA sessions that was inactive for 5 minutes.
I created some of my own session reuse logic for the SharpSVN "SvnRemoteSession" which is a wrapper around the RA sessions. For "connectionless" sessions like http(s), I just reuse them, while for "connection" sessions (svn:// and especially svn+ssh://), is end a small "ping" in form of an "stat" request to validate that the session is still active. This gives me a high reliability, and a huge speedup over creating a new session (especially for ssh connections), but still have the guarantee that I won't hand out sessions which are totally broken.
Maybe some similar scheme can be used here, possibly with a small time period after last successful usage where the revalidation may be skipped.
Best regards
Markus Schaber
CODESYS® a trademark of 3S-Smart Software Solutions GmbH
Inspiring Automation Solutions
3S-Smart Software Solutions GmbH
Dipl.-Inf. Markus Schaber | Product Development Core Technology
Memminger Str. 151 | 87439 Kempten | Germany
Tel. +49-831-54031-979 | Fax +49-831-54031-50
E-Mail: m.schaber@codesys.com | Web: http://www.codesys.com | CODESYS store: http://store.codesys.com
CODESYS forum: http://forum.codesys.com
Managing Directors: Dipl.Inf. Dieter Hess, Dipl.Inf. Manfred Werner | Trade register: Kempten HRB 6186 | Tax ID No.: DE 167014915
This e-mail may contain confidential and/or privileged information. If you are not the intended recipient (or have received
this e-mail in error) please notify the sender immediately and destroy this e-mail. Any unauthorised copying, disclosure
or distribution of the material in this e-mail is strictly forbidden.
Re: Merge ra-reuse-session branch to trunk or not?
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 18 September 2015 at 01:50, Stefan Fuhrmann
<st...@wandisco.com> wrote:
> On Fri, Sep 18, 2015 at 12:08 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>
>> On 17 September 2015 at 21:53, Philip Martin <ph...@wandisco.com>
>> wrote:
>> > Ivan Zhakov <iv...@visualsvn.com> writes:
>> >
[...]
>> > Detect the error from a broken RA
>> > session and create another? Track the time when the session was last
>> > used? Something else?
>> >
>> Current implementation tracks last time when session was used and do
>> not reuse RA sessions that was inactive for 5 minutes.
>
>
> My 2 cents: I wonder whether 5 minutes are a good default
> or whether 0.5 .. 1 minute wouldn't be a better one. A lower
> value allows for faster recovery from intermittent failures.
>
> The typical use-cases for this feature are c/o with many
> svn:externals and interactive GUIs like TSVN's repo browser.
> Both should be fine with timeouts of about 1 minute. But this
> is just a small point that sprang to my attention and I don't feel
> like we need to have a discussion on this topic right now.
>
Fully agree. I changed expiry timeout to 1 minute in r1703803.
Actually I thought that RA expiry timeout was 5 *seconds* :)
--
Ivan Zhakov
Re: Merge ra-reuse-session branch to trunk or not?
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Fri, Sep 18, 2015 at 12:08 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 17 September 2015 at 21:53, Philip Martin <ph...@wandisco.com>
> wrote:
> > Ivan Zhakov <iv...@visualsvn.com> writes:
> >
> >> I think now is good moment to discuss whether we should merge
> >> ra-reuse-session [1] branch to trunk or not: it's better to merge such
> >> branch in the beginning of release cycle, to have more time to test
> >> and dogfood.
> >
> > +1 to merge.
>
+1 to merge for dogfooding. I have not looked at the branch
but I'm +1 on the general concept. So, let's start looking for
the real issues now simply by using it ...
> >> Cons:
> >> - In makes behavior less stable. RA session pool doesn't reuse
> >> sessions that was unused for some time to avoid timeout issues
> >> - There is the chance that we will try to reuse 'broken' RA session
> >> due the bug and operation will fail
> >
> > Do you have a plan to fix this?
> I don't have specific to fix bug that didn't happen. But if we got one
> we have two directions:
> - Do not release RA session back to pool in specific case where we get it
> broken
> - Make RA session more resilent to errors. There is no reason why
> ra_svn cannot reconnect after TCP connection times out or something.
>
> > Detect the error from a broken RA
> > session and create another? Track the time when the session was last
> > used? Something else?
> >
> Current implementation tracks last time when session was used and do
> not reuse RA sessions that was inactive for 5 minutes.
>
My 2 cents: I wonder whether 5 minutes are a good default
or whether 0.5 .. 1 minute wouldn't be a better one. A lower
value allows for faster recovery from intermittent failures.
The typical use-cases for this feature are c/o with many
svn:externals and interactive GUIs like TSVN's repo browser.
Both should be fine with timeouts of about 1 minute. But this
is just a small point that sprang to my attention and I don't feel
like we need to have a discussion on this topic right now.
-- Stefan^2.
Re: Merge ra-reuse-session branch to trunk or not?
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 17 September 2015 at 21:53, Philip Martin <ph...@wandisco.com> wrote:
> Ivan Zhakov <iv...@visualsvn.com> writes:
>
>> I think now is good moment to discuss whether we should merge
>> ra-reuse-session [1] branch to trunk or not: it's better to merge such
>> branch in the beginning of release cycle, to have more time to test
>> and dogfood.
>
> +1 to merge.
>
>> Cons:
>> - In makes behavior less stable. RA session pool doesn't reuse
>> sessions that was unused for some time to avoid timeout issues
>> - There is the chance that we will try to reuse 'broken' RA session
>> due the bug and operation will fail
>
> Do you have a plan to fix this?
I don't have specific to fix bug that didn't happen. But if we got one
we have two directions:
- Do not release RA session back to pool in specific case where we get it broken
- Make RA session more resilent to errors. There is no reason why
ra_svn cannot reconnect after TCP connection times out or something.
> Detect the error from a broken RA
> session and create another? Track the time when the session was last
> used? Something else?
>
Current implementation tracks last time when session was used and do
not reuse RA sessions that was inactive for 5 minutes.
--
Ivan Zhakov
Re: Merge ra-reuse-session branch to trunk or not?
Posted by Philip Martin <ph...@wandisco.com>.
Ivan Zhakov <iv...@visualsvn.com> writes:
> I think now is good moment to discuss whether we should merge
> ra-reuse-session [1] branch to trunk or not: it's better to merge such
> branch in the beginning of release cycle, to have more time to test
> and dogfood.
+1 to merge.
> Cons:
> - In makes behavior less stable. RA session pool doesn't reuse
> sessions that was unused for some time to avoid timeout issues
> - There is the chance that we will try to reuse 'broken' RA session
> due the bug and operation will fail
Do you have a plan to fix this? Detect the error from a broken RA
session and create another? Track the time when the session was last
used? Something else?
--
Philip Martin
WANdisco
Re: Merge ra-reuse-session branch to trunk or not?
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 21 September 2015 at 12:53, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 21 September 2015 at 12:47, Branko Čibej <br...@apache.org> wrote:
>> On 20.09.2015 09:40, Ivan Zhakov wrote:
>>> On 20 September 2015 at 00:53, Branko Čibej <br...@apache.org> wrote:
>>>> On 19.09.2015 19:20, Ivan Zhakov wrote:
>>>>> On 19 September 2015 at 17:24, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>>>>> On 19 September 2015 at 14:03, Branko Čibej <br...@apache.org> wrote:
>>>>>>> On 19.09.2015 13:12, Ivan Zhakov wrote:
>>>>>>>> On 18 September 2015 at 12:49, Stefan Sperling <st...@elego.de> wrote:
>>>>>>>>> On Thu, Sep 17, 2015 at 05:41:41PM +0200, Ivan Zhakov wrote:
>>>>>>>>>> That branch is complete and ready for merging, but I'm still not sure
>>>>>>>>>> whether we should merge it or not.
>>>>>>>>> I think we should merge it to trunk now.
>>>>>>>>>
>>>>>>>>> I don't think this branch can improve much further unless we start
>>>>>>>>> exercising the code ourselves to see how well it's working for us.
>>>>>>>>>
>>>>>>>>> ANd I believe it's hard to tell whether these changes provide an
>>>>>>>>> actual benefit in practice without running the code for a while.
>>>>>>>>>
>>>>>>>>> I like the debug and profiling functionality.
>>>>>>>>> This should make it easy to tune the system going forward.
>>>>>>>> Ok. It seems people here generally support 'reuse-ra-session' branch
>>>>>>>> concept. So I'm going to merge this branch to trunk and see how it
>>>>>>>> will work.
>>>>>>> +1
>>>>>> Merged to trunk in r1704048.
>>>>>>
>>>>> The r1704048 broke JavaHL tests.
>>>>>
>>>>> This happens because JavaHL bindings changes content of AUTH_BATON
>>>>> field in svn_client_ctx_t between diferent svn_client_*() invocations.
>>>>> While RA session in RA session pool references AUTH_BATON from first
>>>>> invocation.
>>>>>
>>>>> The most interesting question is it allowed by our API or not?
>>>> Unfortunately, I'd say it is because none of the API docs say otherwise.
>>>> Or at least I can't find any.
>>>>
>>> I also couldn't find any of them :(
>>>
>>>>> If it's not allowed we just need to fix JavaHL to use the same AUTH_BATON.
>>>> I think the most interesting question here is: why is JavaHL doing this
>>>> in the first place? I have to confess I've no idea, offhand.
>>>>
>>> I also have no idea, but I was sort of hoping that you could provide
>>> some insight on this part :)
>>>
>>> Anyway, as we are doing this kind of things ourselves in JavaHL, there
>>> could be other API users that are also doing it, and we will probably
>>> break them unless we revv the API.
>>
>> Here's one more data point that has nothing to do with JavaHL: The
>> svn-x64-macosx-bdb builder started failing after the merge:
>>
>> https://ci.apache.org/builders/svn-x64-macosx-bdb/builds/253/steps/Test%20ra_local%2Bbdb/logs/faillog
>>
> Yup, I've seen it. This one seems to be caused by too many BDB handles
> opened due parallel run or something. Anyway I'm currently working on
> revertng r1704048: it's clear that code is not ready for prime time
> and it's not obvious how to fix even known problems.
>
I've reverted r1704048 in r1704255. I'll revive 'reuse-ra-session'
branch and attempt to fix these problems there someday.
--
Ivan Zhakov
RE: Merge ra-reuse-session branch to trunk or not?
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Ivan Zhakov [mailto:ivan@visualsvn.com]
> Sent: maandag 21 september 2015 11:53
> To: Branko Čibej <br...@apache.org>
> Cc: dev@subversion.apache.org
> Subject: Re: Merge ra-reuse-session branch to trunk or not?
> >
> > Here's one more data point that has nothing to do with JavaHL: The
> > svn-x64-macosx-bdb builder started failing after the merge:
> >
> > https://ci.apache.org/builders/svn-x64-macosx-
> bdb/builds/253/steps/Test%20ra_local%2Bbdb/logs/faillog
> >
> Yup, I've seen it. This one seems to be caused by too many BDB handles
> opened due parallel run or something. Anyway I'm currently working on
> revertng r1704048: it's clear that code is not ready for prime time
> and it's not obvious how to fix even known problems.
This test copies the repository to do some relocation testing. It does this with the BDB handle cached via the ra session.
I don't think this test is really a valid datapoint to veto the change on. This test is just a hack to reproduce an earlier database inconsistency.
Bert
Re: Merge ra-reuse-session branch to trunk or not?
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 21 September 2015 at 12:47, Branko Čibej <br...@apache.org> wrote:
> On 20.09.2015 09:40, Ivan Zhakov wrote:
>> On 20 September 2015 at 00:53, Branko Čibej <br...@apache.org> wrote:
>>> On 19.09.2015 19:20, Ivan Zhakov wrote:
>>>> On 19 September 2015 at 17:24, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>>>> On 19 September 2015 at 14:03, Branko Čibej <br...@apache.org> wrote:
>>>>>> On 19.09.2015 13:12, Ivan Zhakov wrote:
>>>>>>> On 18 September 2015 at 12:49, Stefan Sperling <st...@elego.de> wrote:
>>>>>>>> On Thu, Sep 17, 2015 at 05:41:41PM +0200, Ivan Zhakov wrote:
>>>>>>>>> That branch is complete and ready for merging, but I'm still not sure
>>>>>>>>> whether we should merge it or not.
>>>>>>>> I think we should merge it to trunk now.
>>>>>>>>
>>>>>>>> I don't think this branch can improve much further unless we start
>>>>>>>> exercising the code ourselves to see how well it's working for us.
>>>>>>>>
>>>>>>>> ANd I believe it's hard to tell whether these changes provide an
>>>>>>>> actual benefit in practice without running the code for a while.
>>>>>>>>
>>>>>>>> I like the debug and profiling functionality.
>>>>>>>> This should make it easy to tune the system going forward.
>>>>>>> Ok. It seems people here generally support 'reuse-ra-session' branch
>>>>>>> concept. So I'm going to merge this branch to trunk and see how it
>>>>>>> will work.
>>>>>> +1
>>>>> Merged to trunk in r1704048.
>>>>>
>>>> The r1704048 broke JavaHL tests.
>>>>
>>>> This happens because JavaHL bindings changes content of AUTH_BATON
>>>> field in svn_client_ctx_t between diferent svn_client_*() invocations.
>>>> While RA session in RA session pool references AUTH_BATON from first
>>>> invocation.
>>>>
>>>> The most interesting question is it allowed by our API or not?
>>> Unfortunately, I'd say it is because none of the API docs say otherwise.
>>> Or at least I can't find any.
>>>
>> I also couldn't find any of them :(
>>
>>>> If it's not allowed we just need to fix JavaHL to use the same AUTH_BATON.
>>> I think the most interesting question here is: why is JavaHL doing this
>>> in the first place? I have to confess I've no idea, offhand.
>>>
>> I also have no idea, but I was sort of hoping that you could provide
>> some insight on this part :)
>>
>> Anyway, as we are doing this kind of things ourselves in JavaHL, there
>> could be other API users that are also doing it, and we will probably
>> break them unless we revv the API.
>
> Here's one more data point that has nothing to do with JavaHL: The
> svn-x64-macosx-bdb builder started failing after the merge:
>
> https://ci.apache.org/builders/svn-x64-macosx-bdb/builds/253/steps/Test%20ra_local%2Bbdb/logs/faillog
>
Yup, I've seen it. This one seems to be caused by too many BDB handles
opened due parallel run or something. Anyway I'm currently working on
revertng r1704048: it's clear that code is not ready for prime time
and it's not obvious how to fix even known problems.
--
Ivan Zhakov
Re: Merge ra-reuse-session branch to trunk or not?
Posted by Branko Čibej <br...@apache.org>.
On 20.09.2015 09:40, Ivan Zhakov wrote:
> On 20 September 2015 at 00:53, Branko Čibej <br...@apache.org> wrote:
>> On 19.09.2015 19:20, Ivan Zhakov wrote:
>>> On 19 September 2015 at 17:24, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>>> On 19 September 2015 at 14:03, Branko Čibej <br...@apache.org> wrote:
>>>>> On 19.09.2015 13:12, Ivan Zhakov wrote:
>>>>>> On 18 September 2015 at 12:49, Stefan Sperling <st...@elego.de> wrote:
>>>>>>> On Thu, Sep 17, 2015 at 05:41:41PM +0200, Ivan Zhakov wrote:
>>>>>>>> That branch is complete and ready for merging, but I'm still not sure
>>>>>>>> whether we should merge it or not.
>>>>>>> I think we should merge it to trunk now.
>>>>>>>
>>>>>>> I don't think this branch can improve much further unless we start
>>>>>>> exercising the code ourselves to see how well it's working for us.
>>>>>>>
>>>>>>> ANd I believe it's hard to tell whether these changes provide an
>>>>>>> actual benefit in practice without running the code for a while.
>>>>>>>
>>>>>>> I like the debug and profiling functionality.
>>>>>>> This should make it easy to tune the system going forward.
>>>>>> Ok. It seems people here generally support 'reuse-ra-session' branch
>>>>>> concept. So I'm going to merge this branch to trunk and see how it
>>>>>> will work.
>>>>> +1
>>>> Merged to trunk in r1704048.
>>>>
>>> The r1704048 broke JavaHL tests.
>>>
>>> This happens because JavaHL bindings changes content of AUTH_BATON
>>> field in svn_client_ctx_t between diferent svn_client_*() invocations.
>>> While RA session in RA session pool references AUTH_BATON from first
>>> invocation.
>>>
>>> The most interesting question is it allowed by our API or not?
>> Unfortunately, I'd say it is because none of the API docs say otherwise.
>> Or at least I can't find any.
>>
> I also couldn't find any of them :(
>
>>> If it's not allowed we just need to fix JavaHL to use the same AUTH_BATON.
>> I think the most interesting question here is: why is JavaHL doing this
>> in the first place? I have to confess I've no idea, offhand.
>>
> I also have no idea, but I was sort of hoping that you could provide
> some insight on this part :)
>
> Anyway, as we are doing this kind of things ourselves in JavaHL, there
> could be other API users that are also doing it, and we will probably
> break them unless we revv the API.
Here's one more data point that has nothing to do with JavaHL: The
svn-x64-macosx-bdb builder started failing after the merge:
https://ci.apache.org/builders/svn-x64-macosx-bdb/builds/253/steps/Test%20ra_local%2Bbdb/logs/faillog
-- Brane
Re: Merge ra-reuse-session branch to trunk or not?
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 20 September 2015 at 00:53, Branko Čibej <br...@apache.org> wrote:
> On 19.09.2015 19:20, Ivan Zhakov wrote:
>> On 19 September 2015 at 17:24, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>> On 19 September 2015 at 14:03, Branko Čibej <br...@apache.org> wrote:
>>>> On 19.09.2015 13:12, Ivan Zhakov wrote:
>>>>> On 18 September 2015 at 12:49, Stefan Sperling <st...@elego.de> wrote:
>>>>>> On Thu, Sep 17, 2015 at 05:41:41PM +0200, Ivan Zhakov wrote:
>>>>>>> That branch is complete and ready for merging, but I'm still not sure
>>>>>>> whether we should merge it or not.
>>>>>> I think we should merge it to trunk now.
>>>>>>
>>>>>> I don't think this branch can improve much further unless we start
>>>>>> exercising the code ourselves to see how well it's working for us.
>>>>>>
>>>>>> ANd I believe it's hard to tell whether these changes provide an
>>>>>> actual benefit in practice without running the code for a while.
>>>>>>
>>>>>> I like the debug and profiling functionality.
>>>>>> This should make it easy to tune the system going forward.
>>>>> Ok. It seems people here generally support 'reuse-ra-session' branch
>>>>> concept. So I'm going to merge this branch to trunk and see how it
>>>>> will work.
>>>> +1
>>> Merged to trunk in r1704048.
>>>
>> The r1704048 broke JavaHL tests.
>>
>> This happens because JavaHL bindings changes content of AUTH_BATON
>> field in svn_client_ctx_t between diferent svn_client_*() invocations.
>> While RA session in RA session pool references AUTH_BATON from first
>> invocation.
>>
>> The most interesting question is it allowed by our API or not?
>
> Unfortunately, I'd say it is because none of the API docs say otherwise.
> Or at least I can't find any.
>
I also couldn't find any of them :(
>> If it's not allowed we just need to fix JavaHL to use the same AUTH_BATON.
>
> I think the most interesting question here is: why is JavaHL doing this
> in the first place? I have to confess I've no idea, offhand.
>
I also have no idea, but I was sort of hoping that you could provide
some insight on this part :)
Anyway, as we are doing this kind of things ourselves in JavaHL, there
could be other API users that are also doing it, and we will probably
break them unless we revv the API.
--
Ivan Zhakov
Re: Merge ra-reuse-session branch to trunk or not?
Posted by Branko Čibej <br...@apache.org>.
On 19.09.2015 19:20, Ivan Zhakov wrote:
> On 19 September 2015 at 17:24, Ivan Zhakov <iv...@visualsvn.com> wrote:
>> On 19 September 2015 at 14:03, Branko Čibej <br...@apache.org> wrote:
>>> On 19.09.2015 13:12, Ivan Zhakov wrote:
>>>> On 18 September 2015 at 12:49, Stefan Sperling <st...@elego.de> wrote:
>>>>> On Thu, Sep 17, 2015 at 05:41:41PM +0200, Ivan Zhakov wrote:
>>>>>> That branch is complete and ready for merging, but I'm still not sure
>>>>>> whether we should merge it or not.
>>>>> I think we should merge it to trunk now.
>>>>>
>>>>> I don't think this branch can improve much further unless we start
>>>>> exercising the code ourselves to see how well it's working for us.
>>>>>
>>>>> ANd I believe it's hard to tell whether these changes provide an
>>>>> actual benefit in practice without running the code for a while.
>>>>>
>>>>> I like the debug and profiling functionality.
>>>>> This should make it easy to tune the system going forward.
>>>> Ok. It seems people here generally support 'reuse-ra-session' branch
>>>> concept. So I'm going to merge this branch to trunk and see how it
>>>> will work.
>>> +1
>> Merged to trunk in r1704048.
>>
> The r1704048 broke JavaHL tests.
>
> This happens because JavaHL bindings changes content of AUTH_BATON
> field in svn_client_ctx_t between diferent svn_client_*() invocations.
> While RA session in RA session pool references AUTH_BATON from first
> invocation.
>
> The most interesting question is it allowed by our API or not?
Unfortunately, I'd say it is because none of the API docs say otherwise.
Or at least I can't find any.
> If it's not allowed we just need to fix JavaHL to use the same AUTH_BATON.
I think the most interesting question here is: why is JavaHL doing this
in the first place? I have to confess I've no idea, offhand.
-- Brane
Re: Merge ra-reuse-session branch to trunk or not?
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 19 September 2015 at 21:17, Bert Huijben <be...@qqmail.nl> wrote:
> You wouldn't be able to check the id of the old auth baton once the pool is
> cleaned...
>
I can save old auth baton id in cache_entry once I pass it to
svn_ra_open4() and compare it to the current value in
svn_client_ctx_t.
[..]
> It is possible to attach a pool cleanup handler to see when the auth baton
> is cleared, but I don't like that solution either.
>
I considered this approach, but decided even didn't list it as an
option since it's very fragile and introduce complex dependencies
between objects.
> I agree with Ivan: We should check if the javahl trickery is valid. (We can
> fix javahl of course, but perhaps other users do the same thing)
>
> We probably never described limitations...
>
Exactly. Proper solution would be to expose this limitation as part of
our API by accepting AUTH_BATON and TUNNEL_FUNC as parameter for
svn_client_context_create() function and never expose it in
svn_client_ctx2_t. But this will require to revv all svn_client_*()
functions. I'm not sure that it worth the effort, but I can do it if
everybody agree that this is good and right thing.
--
Ivan Zhakov
RE: Merge ra-reuse-session branch to trunk or not?
Posted by Bert Huijben <be...@qqmail.nl>.
You wouldn't be able to check the id of the old auth baton once the pool is cleaned...
Libsvn_client could do some magic but this would have to be done in every place where we create/fetch Ra sessions. This makes it even more of a hack than the current code, while the idea was to clean things up by moving some of the code in the Ra sessions.
(Thanks autocorrect for capitalizing Ra)
It is possible to attach a pool cleanup handler to see when the auth baton is cleared, but I don't like that solution either.
I agree with Ivan: We should check if the javahl trickery is valid. (We can fix javahl of course, but perhaps other users do the same thing)
We probably never described limitations...
Bert
-----Original Message-----
From: "Evgeny Kotkov" <ev...@visualsvn.com>
Sent: 19-9-2015 19:49
To: "Ivan Zhakov" <iv...@visualsvn.com>
Cc: "Branko Čibej" <br...@apache.org>; "Subversion Development" <de...@subversion.apache.org>
Subject: Re: Merge ra-reuse-session branch to trunk or not?
Ivan Zhakov <iv...@visualsvn.com> writes:
> Otherwise, here are my ideas at the moment:
> 1. Add svn_client_ctx2_t with svn_client_context_create3() that's
> accept AUTH_BATON as parameter. This will require revv all
> svn_client_*() functions to create new svn_client_ctx2_t() on every
> invocation of deprecated function.
>
> 2. Add svn_ra_set_auth_baton() function to change AUTH_BATON in
> existing RA session. But in this case RA session implementation should
> re-authenticate after every reuse because new AUTH_BATON may have
> different credentials.
>
> 3. Revert r1704048 and wait until next hackathon for better ideas :)
As a random idea:
4. Make each svn_auth_baton_t have a unique ID generated upon creation with
svn_auth_open(). Place this ID alongside with the baton in the RA session
pool. Open a new RA session in case the baton's ID does not match the ID
within the cached session — i.e., do not try to reuse the session if the
auth baton IDs do not match.
Regards,
Evgeny Kotkov
Re: Merge ra-reuse-session branch to trunk or not?
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 19 September 2015 at 19:49, Evgeny Kotkov
<ev...@visualsvn.com> wrote:
> Ivan Zhakov <iv...@visualsvn.com> writes:
>
>> Otherwise, here are my ideas at the moment:
>> 1. Add svn_client_ctx2_t with svn_client_context_create3() that's
>> accept AUTH_BATON as parameter. This will require revv all
>> svn_client_*() functions to create new svn_client_ctx2_t() on every
>> invocation of deprecated function.
>>
>> 2. Add svn_ra_set_auth_baton() function to change AUTH_BATON in
>> existing RA session. But in this case RA session implementation should
>> re-authenticate after every reuse because new AUTH_BATON may have
>> different credentials.
>>
>> 3. Revert r1704048 and wait until next hackathon for better ideas :)
>
> As a random idea:
>
> 4. Make each svn_auth_baton_t have a unique ID generated upon creation with
> svn_auth_open(). Place this ID alongside with the baton in the RA session
> pool. Open a new RA session in case the baton's ID does not match the ID
> within the cached session — i.e., do not try to reuse the session if the
> auth baton IDs do not match.
>
I've tried this idea and it seems to solve problem with AUTH_BATON,
but then I got similar problem with OPEN_TUNNEL_FUNC callbacks: JavaHL
implementation is not ready that tunnel created but JavaHL implemented
callback assumes that created tunnel will not be used after
svn_client_*() call complete.
Given all these problems I'm inclined to revert r1704048 and continue
fixing this problems in branch to keep trunk in stable state. It's
very unlikely that I will be able to solve these problems in next few
weeks.
--
Ivan Zhakov
Re: Merge ra-reuse-session branch to trunk or not?
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Ivan Zhakov <iv...@visualsvn.com> writes:
> Otherwise, here are my ideas at the moment:
> 1. Add svn_client_ctx2_t with svn_client_context_create3() that's
> accept AUTH_BATON as parameter. This will require revv all
> svn_client_*() functions to create new svn_client_ctx2_t() on every
> invocation of deprecated function.
>
> 2. Add svn_ra_set_auth_baton() function to change AUTH_BATON in
> existing RA session. But in this case RA session implementation should
> re-authenticate after every reuse because new AUTH_BATON may have
> different credentials.
>
> 3. Revert r1704048 and wait until next hackathon for better ideas :)
As a random idea:
4. Make each svn_auth_baton_t have a unique ID generated upon creation with
svn_auth_open(). Place this ID alongside with the baton in the RA session
pool. Open a new RA session in case the baton's ID does not match the ID
within the cached session — i.e., do not try to reuse the session if the
auth baton IDs do not match.
Regards,
Evgeny Kotkov
Re: Merge ra-reuse-session branch to trunk or not?
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 19 September 2015 at 17:24, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 19 September 2015 at 14:03, Branko Čibej <br...@apache.org> wrote:
>> On 19.09.2015 13:12, Ivan Zhakov wrote:
>>> On 18 September 2015 at 12:49, Stefan Sperling <st...@elego.de> wrote:
>>>> On Thu, Sep 17, 2015 at 05:41:41PM +0200, Ivan Zhakov wrote:
>>>>> That branch is complete and ready for merging, but I'm still not sure
>>>>> whether we should merge it or not.
>>>> I think we should merge it to trunk now.
>>>>
>>>> I don't think this branch can improve much further unless we start
>>>> exercising the code ourselves to see how well it's working for us.
>>>>
>>>> ANd I believe it's hard to tell whether these changes provide an
>>>> actual benefit in practice without running the code for a while.
>>>>
>>>> I like the debug and profiling functionality.
>>>> This should make it easy to tune the system going forward.
>>> Ok. It seems people here generally support 'reuse-ra-session' branch
>>> concept. So I'm going to merge this branch to trunk and see how it
>>> will work.
>>
>> +1
> Merged to trunk in r1704048.
>
The r1704048 broke JavaHL tests.
This happens because JavaHL bindings changes content of AUTH_BATON
field in svn_client_ctx_t between diferent svn_client_*() invocations.
While RA session in RA session pool references AUTH_BATON from first
invocation.
The most interesting question is it allowed by our API or not?
If it's not allowed we just need to fix JavaHL to use the same AUTH_BATON.
Otherwise, here are my ideas at the moment:
1. Add svn_client_ctx2_t with svn_client_context_create3() that's
accept AUTH_BATON as parameter. This will require revv all
svn_client_*() functions to create new svn_client_ctx2_t() on every
invocation of deprecated function.
2. Add svn_ra_set_auth_baton() function to change AUTH_BATON in
existing RA session. But in this case RA session implementation should
re-authenticate after every reuse because new AUTH_BATON may have
different credentials.
3. Revert r1704048 and wait until next hackathon for better ideas :)
--
Ivan Zhakov
Re: Merge ra-reuse-session branch to trunk or not?
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 19 September 2015 at 14:03, Branko Čibej <br...@apache.org> wrote:
> On 19.09.2015 13:12, Ivan Zhakov wrote:
>> On 18 September 2015 at 12:49, Stefan Sperling <st...@elego.de> wrote:
>>> On Thu, Sep 17, 2015 at 05:41:41PM +0200, Ivan Zhakov wrote:
>>>> That branch is complete and ready for merging, but I'm still not sure
>>>> whether we should merge it or not.
>>> I think we should merge it to trunk now.
>>>
>>> I don't think this branch can improve much further unless we start
>>> exercising the code ourselves to see how well it's working for us.
>>>
>>> ANd I believe it's hard to tell whether these changes provide an
>>> actual benefit in practice without running the code for a while.
>>>
>>> I like the debug and profiling functionality.
>>> This should make it easy to tune the system going forward.
>> Ok. It seems people here generally support 'reuse-ra-session' branch
>> concept. So I'm going to merge this branch to trunk and see how it
>> will work.
>
> +1
Merged to trunk in r1704048.
--
Ivan Zhakov
Re: Merge ra-reuse-session branch to trunk or not?
Posted by Branko Čibej <br...@apache.org>.
On 19.09.2015 13:12, Ivan Zhakov wrote:
> On 18 September 2015 at 12:49, Stefan Sperling <st...@elego.de> wrote:
>> On Thu, Sep 17, 2015 at 05:41:41PM +0200, Ivan Zhakov wrote:
>>> That branch is complete and ready for merging, but I'm still not sure
>>> whether we should merge it or not.
>> I think we should merge it to trunk now.
>>
>> I don't think this branch can improve much further unless we start
>> exercising the code ourselves to see how well it's working for us.
>>
>> ANd I believe it's hard to tell whether these changes provide an
>> actual benefit in practice without running the code for a while.
>>
>> I like the debug and profiling functionality.
>> This should make it easy to tune the system going forward.
> Ok. It seems people here generally support 'reuse-ra-session' branch
> concept. So I'm going to merge this branch to trunk and see how it
> will work.
+1
Re: Merge ra-reuse-session branch to trunk or not?
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 18 September 2015 at 12:49, Stefan Sperling <st...@elego.de> wrote:
> On Thu, Sep 17, 2015 at 05:41:41PM +0200, Ivan Zhakov wrote:
>> That branch is complete and ready for merging, but I'm still not sure
>> whether we should merge it or not.
>
> I think we should merge it to trunk now.
>
> I don't think this branch can improve much further unless we start
> exercising the code ourselves to see how well it's working for us.
>
> ANd I believe it's hard to tell whether these changes provide an
> actual benefit in practice without running the code for a while.
>
> I like the debug and profiling functionality.
> This should make it easy to tune the system going forward.
Ok. It seems people here generally support 'reuse-ra-session' branch
concept. So I'm going to merge this branch to trunk and see how it
will work.
--
Ivan Zhakov
Re: Merge ra-reuse-session branch to trunk or not?
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Sep 17, 2015 at 05:41:41PM +0200, Ivan Zhakov wrote:
> That branch is complete and ready for merging, but I'm still not sure
> whether we should merge it or not.
I think we should merge it to trunk now.
I don't think this branch can improve much further unless we start
exercising the code ourselves to see how well it's working for us.
ANd I believe it's hard to tell whether these changes provide an
actual benefit in practice without running the code for a while.
I like the debug and profiling functionality.
This should make it easy to tune the system going forward.