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

Michal Židek mzidek at redhat.com
Fri Dec 5 13:07:25 UTC 2014


On 12/04/2014 05:47 PM, Pavel Březina wrote:
> On 12/03/2014 05:27 PM, 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.
>>
>> The patch is indeed an improvement in readability. Just
>> add this change to avoid compiler warnings
>>
>> --- a/src/providers/dp_ptask.c
>> +++ b/src/providers/dp_ptask.c
>> @@ -220,6 +220,10 @@ static void be_ptask_schedule(struct be_ptask *task,
>>               task->period *= 2;
>>           }
>>           break;
>> +    default:
>> +        DEBUG(SSSDBG_FATAL_FAILURE,
>> +              "Unknown delay_type value: %d\n", delay_type);
>> +        return;
>>       }
>>
>>       /* add random offset */
>>
>> I think the level can be FATAL, because it should happen only
>> under influence of some dark magic.
>>
>> Otherwise the patch looks good.
>>
>> Michal
>
> Thank you for the review. The only warning I saw is "delay may be
> uninitialized" which is false positive... I think it is better to
> initialize the variable instead of using default branch, since I would
> like to get warning if we add new delay type and forget to alter this
> switch.
>
> New patch is attached, also with unit tests.
>

Thank you.

ACK.

Michal




More information about the sssd-devel mailing list