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

Michal Židek mzidek at redhat.com
Wed Dec 3 16:27:54 UTC 2014


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



More information about the sssd-devel mailing list