Re: [PATCH] MEDIUM: backend: Allow redispatch on retry intervals

2015-05-21 Thread Willy Tarreau 
Hi Joseph,

On Thu, May 21, 2015 at 10:50:17AM -0700, Joseph Lynch wrote:
 Hello Willy,
 
 On Sat, May 16, 2015 at 2:05 AM, Willy Tarreau w...@1wt.eu wrote:
  I moved the order of the comparisons around a little bit to ensure
  that the redispatch_after variable would be defined (namely if
  PR_O_REDISP is set then redispatch_after must have been set to -1).
 
  I think that we'll be able to get rid of the option after that, as
  a cleanup resulting from your work.
 
 I believe in this iteration of the patch I have initialized the
 redispatch_after variable so that this cleanup is easier in the
 future.

Perfect, I've applied it, thank you.

 It should just be an inversion of the check but against
 redispatch_after. If you like I can follow up this patch with that
 cleanup.

As you like!

Thanks!
Willy

Re: [PATCH] MEDIUM: backend: Allow redispatch on retry intervals

2015-05-16 Thread Willy Tarreau 
Hi Joseph,

On Thu, May 14, 2015 at 09:06:23AM -0700, Joseph Lynch wrote:
 The attached patch allows HAProxy to redispatch on retry intervals,
 e.g. every retry (1), on the last retry (-1), or every 3rd retry (3).
 The idea is to not have to wait for the final retry before
 redispatching.
 
 I moved the order of the comparisons around a little bit to ensure
 that the redispatch_after variable would be defined (namely if
 PR_O_REDISP is set then redispatch_after must have been set to -1).

I think that we'll be able to get rid of the option after that, as
a cleanup resulting from your work.

 I
 did notice that with round robin load balancing we can immediately
 redispatch due to the other branch on the || conditional in the status
 quo, is that intentional? For testing I had to use leastconn.

That reminded me something, so I looked and found this commit which
did this :

  commit 33a14e515bdbb02d8193bd870f3b659243a851f9
  Author: Willy Tarreau w...@1wt.eu
  Date:   Fri Jun 13 17:49:40 2014 +0200

MEDIUM: session: redispatch earlier when possible

As discussed with Dmitry Sivachenko, is a server farm has more than one
active server, uses a guaranteed non-determinist algorithm (round robin),
and a connection was initiated from a non-persistent connection, there's
no point insisting to reconnect to the same server after a connect failure,
better redispatch upon the very first retry instead of insisting on the same
server multiple times.

So yes, it's intentional.

 Testing:
 I added a debug line inside the redispatch block, failed 127.0.0.2 and
 then launched some curls against all the frontends. From the debug
 output and the stat interface the option appears to work. If you would
 like me to do more extensive testing let me know and I'm happy to do
 it.

I think your tests are OK. However I don't see the redispatch_after value
being pre-initialized from the defaults value, so I suspect it does not
work across defaults sections.

Please just check for conn_retries in cfgparse.c, everywhere it is
assigned, your new variable must be set as well. Please double-check
this and after that I think it's OK for merging.

Thanks,
Willy