Re: [PATCH] : Prevent redispatcher from selecting the same server

Hi Krzysztof,

I finally found some time to get back to userland :-) I've carefully inspected your patch. Initially I thought that the FWRR change was wrong but it's OK, it's my understanding which was wrong !

However, I'm not sure that I completely get the logic behind the changes in assign_server_and_queue() :

> @@ -1060,9 +1076,8 @@ int assign_server_and_queue(struct session *s)


>  

>  	if (s->flags & SN_ASSIGNED) {

>  		if (s->srv && s->srv->maxqueue > 0 && s->srv->nbpend >= s->srv->maxqueue) {

> -			s->flags &= ~(SN_DIRECT | SN_ASSIGNED | SN_ADDR_SET);
> -			s->srv = NULL;
> -			http_flush_cookie_flags(&s->txn);
> +			/* it's left to the dispatcher to choose a server */
> +			s->flags &= ~(SN_ASSIGNED | SN_ADDR_SET);


>  		} else {

>  			/* a server does not need to be assigned, perhaps because we're in

>  			 * direct mode, or in dispatch or transparent modes where the server

> @@ -1081,7 +1096,22 @@ int assign_server_and_queue(struct session *s)


>  	}

>  

>  	/* a server needs to be assigned */

> +	srv = s->srv;


>  	err = assign_server(s);

> +
> +	if (srv && srv != s->srv) {
> +		/* This session was previously dispatched to another server:
> +		 *  - clear SN_DIRECT
> +		 *  - flush a cookie
> +		 *  - set SN_REDISP if it was successfully redispatched
> +		 */
> +
> +		s->flags &= ~(SN_DIRECT);
> +		http_flush_cookie_flags(&s->txn);
> +		if (srv)
> +			s->flags |= SN_REDISP;
> +	}
> +

I think that the redispatch will not work anymore if we hit srv->maxqueue
for a server : since the SN_DIRECT flag is not cleared anymore, assign_server()
will return exactly the same server. Am I right or am I missing something ?

Then, maybe we have the same problem here, and at every other place where
the redispatch code gets touched :

> @@ -1409,10 +1439,8 @@ int srv_retryable_connect(struct session *t)


>  	}

>  	t->be->redispatches++;

>  

> -	t->flags &= ~(SN_DIRECT | SN_ASSIGNED | SN_ADDR_SET);
> -	t->flags |= SN_REDISP;
> -	t->srv = NULL; /* it's left to the dispatcher to choose a server */
> -	http_flush_cookie_flags(&t->txn);
> +	/* it's left to the dispatcher to choose a server */
> +	t->flags &= ~(SN_ASSIGNED | SN_ADDR_SET);


>  	return 0;

etc...

Please try to explain me if I'm wrong. I think it was Einstein who said that it requires a smarter brain to solve a problem than to create it, which implies that if you do everything using all of your capacities, then you will only have unfixable problems everywhere :-) And I think I've reached that limit now !

BTW, If you think that the LB code needs refactoring, I'm open to discuss about it.

Regards,
Willy Received on 2008/02/10 13:20