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

Lukas Slebodnik lslebodn at redhat.com
Mon Nov 24 15:54:55 UTC 2014


On (24/11/14 16:33), Michal Židek wrote:
>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.
>
It might be based on top of patches "[PATCH] tests: be_ptask"

LS



More information about the sssd-devel mailing list