On 2/14/24 3:45 PM, jean-frederic clere wrote:
> On 2/14/24 08:19, Ruediger Pluem wrote:
>> On 2/9/24 11:59 AM, jean-frederic clere wrote:
>>> Hi,
>>> I have noted to the reset() clean up too much in the balancers:
>>> mod_lbmethod_bybusyness.c for example does:
>>> +++
>>>      for (i = 0; i < balancer->workers->nelts; i++, worker++) {
>>>          (*worker)->s->lbstatus = 0;
>>>          ap_proxy_set_busy_count(*worker, 0); /* BAD */
>>>      }
>>> +++
>>> In fact reset() is called by ap_proxy_initialize_balancer() when a child 
>>> process is created... Resetting the counters messes up
>>> the logic.
>>> Does it make sense to stop calling reset() from 
>>> ap_proxy_initialize_balancer() or is it better to fix all reset()?
>> I am not sure what the original idea / intention of reset was. Until this is 
>> clarified I would not remove the call to reset in
>> ap_proxy_initialize_balancer(). In ap_proxy_sync_balancer the call to it is 
>> guarded by a check for the need_reset flag. Maybe this
>> gives a hint.

Rethinking this. I guess we lack a clear API definition of the reset method of 
a balancer provider and it does not really seem
sensible to reset the stats in case a new child is created. Hence I guess the 
regression risk is rather low when just removing
this call to reset.

> The need_reset is set when a worker is enabled or when the load balancing 
> method is changed.

Isn't that a similar situation to when a worker is added (in respect to the 
point below)?

>>> There is another thing adding a new worker via /balancer-manager/ probably 
>>> requires some kind of reset() otherwise all the load
>>> moves to the new worker which is not the best.... May be calling age() or 
>>> triggering calls to age() can help.
>> Doesn't ap_proxy_sync_balancer take care of this which is called before 
>> processing a request?



Reply via email to