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.