Hi Patrick,
On Thu, Aug 09, 2018 at 06:46:28PM -0400, Patrick Hemmer wrote:
>
> This adds the sample fetch 'be_conn_free([<backend>])'. This sample fetch
> provides the total number of unused connections across available servers
> in the specified backend.
Thanks for writing this one, I recently figured I needed the same for my
build farm :-)
> +be_conn_free([<backend>]) : integer
> + Returns an integer value corresponding to the number of available
> connections
> + across available servers in the backend. Queue slots are not included.
> Backup
> + servers are also not included, unless all other servers are down. If no
> + backend name is specified, the current one is used. But it is also possible
> + to check another backend. It can be used to use a specific farm when the
> + nominal one is full. See also the "be_conn" and "connslots" criteria.
> +
> + OTHER CAVEATS AND NOTES: at this point in time, the code does not take care
> + of dynamic connections. Also, if any of the server maxconn, or maxqueue is
> 0,
> + then this fetch clearly does not make sense, in which case the value
> returned
> + will be -1.
I disagree with making a special case above for maxconn 0. In fact for me
it just means that such a server cannot accept connections, so it simply
doesn't count in the sum, just as if it were not present.
> + px = iterator->proxy;
> + if (!srv_currently_usable(iterator) || ((iterator->flags &
> SRV_F_BACKUP) &&
> + (px->srv_act ||
> + (iterator !=
> px->lbprm.fbck && !(px->options & PR_O_USE_ALL_BK)))))
> + continue;
Please slightly reorder the condition to improve the indent above for
better readability, for example :
if (!srv_currently_usable(iterator) ||
((iterator->flags & SRV_F_BACKUP) &&
(px->srv_act || (iterator != px->lbprm.fbck &&
!(px->options & PR_O_USE_ALL_BK)))))
After checking, I'm OK with the condition :-)
> + if (iterator->maxconn == 0) {
> + /* configuration is stupid */
> + smp->data.u.sint = -1; /* FIXME: stupid value! */
> + return 1;
> + }
> +
> + smp->data.u.sint += (iterator->maxconn - iterator->cur_sess);
Here I'd simply suggest this to replace the whole block :
if (iterator->maxconn > iterator->cur_sess)
smp->data.u.sint += (iterator->maxconn -
iterator->cur_sess);
And then it can properly count available connections through all
available servers, regardless of their individual configuration.
Otherwise I'm fine with this patch.
Thanks,
Willy
