Re: [PATCH] Don't increment server connections too much + fix retries

Hi Krzysztof,

[part 1]

On Mon, Feb 04, 2008 at 02:17:01AM +0100, Krzysztof Oledzki wrote:
> >From 9af493edbe56489524f57c22d58aca0584931c8a Mon Sep 17 00:00:00 2001
> From: Krzysztof Piotr Oledzki <ole#ans.pl>
> Date: Mon, 4 Feb 2008 02:10:09 +0100
> Subject: [BUG] Don't increment server connections too much + fix retries
>
> Commit 98937b875798e10fac671d109355cde29d2a411a while fixing
> one bug introduced another one. With "retries 4" and
> "option redispatch" haproxy tries to connect 4 times to
> one server server and 1 time to a second one. However
> logs showed 5 connections to the first server (the
> last one was counted twice) and 2 to the second.
>
> This patch also fixes srv->retries and be->retries increments.
>
> Now I get: 3 retries and 1 error in a first server (4 cum_sess)
> and 1 error in a second server (1 cum_sess) with:
> retries 4
> option redispatch
>
> and: 4 retries and 1 error (5 cum_sess) with:
> retries 4
>
> So, the number of connections effectively served by a server is:
> srv->cum_sess - srv->failed_conns - srv->retries

[part 2]

> Finally!
>
> However, IMHO the fix from 98937b875798e10fac671d109355cde29d2a411a
> is wrong. Now be->cum_sess < SUM(servers->cum_sess) as with 1 sesssion
> processed by a backed I get 5 "sessions" processed by servers. This
> is very... abnormal. IMHO we should only increment srv->cum_sess
> once and only when a connection was successfully established but we
> need an additional variable: srv->conns.
>
> So, if it is OK I will send an additional patch that renames
> "srv->cum_sess" into "srv->conns" and reintroduces "srv->cum_sess".
>
> The final situation is going to look like this:
> be->cum_sess: connections (sessions) sent from a frontend to a backend
> be->failed_conns: unhandled connections (sessions) by a backend
> be->cum_sess - be->failed_conns: properly handled connections (sessions) by a backend
>
> srv->cum_sess: connections (sessions) successfully sent to a server
> srv->conns: connections sent to a server (both successfully and unsuccessfully)
> srv->conns - srv->failed_conns - srv->retries: connections effectively served by a server
>
> Please note that:
> * SUM(servers->failed_conns) >= be->failed_conns as one session may
> be sent to two servers and both can be down
>
> * SUM(servers->cum_sess) <= be->cum_sess as it is possible for a session
> to be unhandled when it was not possible to connect to a server
>
> Comments?

At first I thought that the attached patch would perform both [part1] and [part2] above. Maybe it was late when I read it, but now that I re-read it, it looks to me like the patch just addresses [part1], which is fine for me to apply. I didn't want to merge a change for #2 along with a fix for #1 at the same time.

If you can confirm my thoughts, I'm happy to merge it.

Thanks!
Willy Received on 2008/02/28 14:51