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