Re: svn commit: r1657797 - /subversion/branches/reuse-ra-session/subversion/libsvn_client/ra_cache.c

On 6 February 2015 at 17:13, Branko Čibej <brane_at_wandisco.com> wrote:
> On 06.02.2015 15:00, Ivan Zhakov wrote:
>> On 6 February 2015 at 16:50, Branko Čibej <brane_at_wandisco.com> wrote:
>>> On 06.02.2015 14:13, ivan_at_apache.org wrote:
>>>> Author: ivan
>>>> Date: Fri Feb 6 13:13:14 2015
>>>> New Revision: 1657797
>>>>
>>>> URL: http://svn.apache.org/r1657797
>>>> Log:
>>>> On the reuse-ra-session branch: Avoid svn_ra_reparent() call and network
>>>> round-trip for svn:// protocol if possible.
>>>>
>>>> * subversion/libsvn_client/ra_cache.c
>>>> (find_session_by_url): Try to find RA session with exact session URL
>>>> match first.
>>>>
>>>> Modified:
>>>> subversion/branches/reuse-ra-session/subversion/libsvn_client/ra_cache.c
>>>>
>>>> Modified: subversion/branches/reuse-ra-session/subversion/libsvn_client/ra_cache.c
>>>> URL: http://svn.apache.org/viewvc/subversion/branches/reuse-ra-session/subversion/libsvn_client/ra_cache.c?rev=1657797&r1=1657796&r2=1657797&view=diff
>>>> ==============================================================================
>>>> --- subversion/branches/reuse-ra-session/subversion/libsvn_client/ra_cache.c (original)
>>>> +++ subversion/branches/reuse-ra-session/subversion/libsvn_client/ra_cache.c Fri Feb 6 13:13:14 2015
>>>> @@ -401,6 +401,24 @@ find_session_by_url(cache_entry_t **cach
>>>> {
>>>> cache_entry_t *cache_entry;
>>>>
>>>> + /* Try to find RA session with exact session URL match first because
>>>> + * the svn_ra_reparent() for svn:// protocol requires network round-trip.
>>>> + */
>>>> + APR_RING_FOREACH(cache_entry, &ra_cache->freelist,
>>>> + cache_entry_t, freelist)
>>>> + {
>>>> + const char *session_url;
>>>> + SVN_ERR_ASSERT(cache_entry->owner_pool == NULL);
>>>> +
>>>> + SVN_ERR(svn_ra_get_session_url(cache_entry->session, &session_url,
>>>> + scratch_pool));
>>>> + if (strcmp(session_url, url) == 0)
>>>> + {
>>>> + *cache_entry_p = cache_entry;
>>>> + return SVN_NO_ERROR;
>>>> + }
>>>> + }
>>> I think this is a mistake. We'll have to do a round-trip anyway to
>>> ensure that the session is in a valid state (no connection expired,
>>> etc.) before we can safely reuse it. The point of the cache is not to
>>> avoid nework round-trips but to avoid expensive connection establishment.
>>>
>> Please let me disagree with you: we end up with optimizations like we
>> currently have in merge.c if obtaining RA session will require
>> additional round-trips comparing to passing some RA sessions around.
>> Ideally svn_ra_reparent() should not require round trip for svn://,
>> but I think we could and workaround/optimization at ra_cache layer for
>> now.
>>
>> Regarding pinging session before reuse: RA session implementions
>> should check connection state if it was inactive for a some period
>> time of time. But this RA protocol specific and ra_serf already has
>> code to reconnect if needed.
>
> Well, right now we have neither automatic reconnection, nor zero-cost
> reparent in ra_svn. If we don't want to add svn_ra__ping, then we need
> at least the former before we can safely merge this branch. But I have
> no idea how to do this.
>
Max idle session age timeout with value like 5 minutes is sufficient
for safe merging this branch IMO. May be I'm missing something but I
don't see scenario when situation will worse than in Subversion 1.8.x,
given that only explicitly released session are returned to RA cache
pool.

We also could reduce max idle session timeout to lower value without
affecting performance.

-- 
Ivan Zhakov