[SSSD] [PATCH] be_ptask: Use first/enabled_delay with backoff feature

Michal Židek mzidek at redhat.com
Mon Nov 24 15:33:52 UTC 2014


On 11/24/2014 12:41 PM, Pavel Březina wrote:
> On 11/21/2014 03:52 PM, Michal Židek wrote:
>> On 11/21/2014 03:17 PM, Pavel Březina wrote:
>>> On 11/21/2014 03:02 PM, Michal Židek wrote:
>>>> On 11/21/2014 12:37 PM, Pavel Březina wrote:
>>>>> On 11/20/2014 07:22 PM, Michal Židek wrote:
>>>>>> Hi,
>>>>>>
>>>>>> it is probably not very intuitive to choose the
>>>>>> 'period' parameter as the initial interval in
>>>>>> the backoff feature (ptask). The first_delay and
>>>>>> enabled_delay are more suitable for this.
>>>>>>
>>>>>> See the attached patch.
>>>>>>
>>>>>> Michal
>>>>>
>>>>> I still don't think this is the best idea. Let's discuss it a little.
>>>>>
>>>>> Can you remind me what is the purpose of the backoff feature? Why
>>>>> was it
>>>>> implemented? AFAIK it was to detect when we can get back online and
>>>>> the
>>>>> randomization was added to spread the load on a server.
>>>>
>>>> We use the ptask API to check if the remote server is still offline. If
>>>> it is (offline), the task is rescheduled with doubled period (+random
>>>> offest).
>>>>
>>>>>
>>>>> I don't like the fact that in the current state the delay parameter is
>>>>> ignored if the backoff is set. And therefore all first delay, enabled
>>>>> delay and period (with your new patch) is ignored.
>>>>
>>>> Now I am confused a little. With the patch that I attached, the
>>>> enabled_delay and first_delay are respected. Only the period
>>>> is ignored (note that the enabled/first_delay are passed as the
>>>> "delay" paramter to the be_ptask_schedule).
>>>
>>> Ah, that is true. Sorry.
>>>
>>> We should not ignore the period though. Imagine that you'll use it for
>>> some task where first/enabled delay differs a lot from period. Or where
>>> first/enabled delay will be zero. We should stick to the original plan
>>> and double the period value.
>>
>> Ok.
>>
>> The originally designed behaviour was following:
>> - shedule execution
>> - if the task was not disabled after last execution, schedule
>>    next execution but double the period
>>
>> If I understand you correctly what you propose is to change it to
>> following:
>> - schedule execution (with first/enabled delay)
>> - if the task was not disabled after last execution:
>>     -schedule next execution using 'period' (if backoff_period was 0)
>>     -schedule next execution with doubled backoff_period
>
> Yes.
>
>> I thought it will be easier to understand the flow if we use
>> one initial delay in backoff and than just double it. But your
>> proposal makes sense. Do you want to prepare the patch or shall
>> I do?
>
> New patch is attached. I removed the backoff_delay field and use period
> directly instead. I think the flow is now pretty straight-forward and
> clean.
>
>>>>> If I understand the purpose of this backoff feature correctly, you
>>>>> want
>>>>> to increase the period after each execution. But in my opinion you
>>>>
>>>> Yes.
>>>>
>>>>> should still obey the first delay and enabled delay and use the
>>>>> *exact*
>>>>> times (even without the random offset IMHO).
>>>>
>>>> We should not ignore the random_offset. That would kill the purpose.
>>>> For example if the server goes off for less than first_delay,
>>>> the next "online check"  will be successful for all clients
>>>> (that is why the randomization is needed even in the first delay).
>>>> It is why the randomization was added.
>>>
>>> OK.
>>>
>>>>
>>>>>
>>>>> Attached is an *untested* patch to illustrate what I have in mind.

The patch does not apply on current master. You probably forgot
to attach some previous patch where you create the private header.

Michal




More information about the sssd-devel mailing list