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?
>>
Regards
Rüdiger
