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