On 04/29/2013 09:49 PM, Simo Sorce wrote:
On Mon, 2013-04-29 at 20:55 +0200, Jakub Hrozek wrote:
On Mon, Apr 29, 2013 at 08:42:30PM +0200, Pavel Březina wrote:
On 04/29/2013 08:12 PM, Simo Sorce wrote:
On Mon, 2013-04-29 at 20:05 +0200, Pavel Březina wrote:
On 04/29/2013 07:52 PM, Simo Sorce wrote:
On Mon, 2013-04-29 at 11:54 +0200, Pavel Březina wrote: > > Ah, yes. If the request returns ERR_STOP_PERIODIC_TASK, the task will > be > completely destroyed, otherwise it will reschedule: when EOK, it will > fire the request in last-execution-time + period, otherwise now + > period. > Wouldn't it be simpler to simply stop on any error, and reschedule only when EOK is returned ? This way the task can bubble up the error all the wya w/o having to mask it to a ERR_STOP_PERIODIC_TASK error at the very end.
No, you want to terminate the periodic task only on fatal error which you can't recover from. E.g. you want to reschedule the task when you can't connect to LDAP, but terminate it if LDAP server doesn't support some extension.
If you return ERR_STOP_PERIODIC_TASK, the task will not be rescheduled. It will be rescheduled on any other error code and logged as appropriate.
I guess the point here is what you want to do by default.
AFAIK none of current periodic tasks ends willingly, only if tevent timer cannot be created. So I think it is OK to anticipate that number of error codes on which you want to continue is far greater than those on which you want to stop.
But then again, we can change it any time, if it won't fit our future needs.
In either case you need to check and remap error codes before returning.
If you define ERR_STOP_PERIODIC_TASK, then you want a continue by default, let's stop only on white listed errors.
If there is a new 'fatal' error you do not catch in whatever function normally converts to ERR_STOP_PERIODIC_TASK. Then the task will be rescheduled.
The other point is that with ERR_STOP_PERIODIC_TASK, you also have to log where you map because it won't bubble up.
I can see the advantages as well though, so I do not have a strong preference at this point, carry on :)
Simo.
I have just amended the design page: https://fedorahosted.org/sssd/wiki/DesignDocs/PeriodicTasks
Fine, let's go ahead with the current design. In 1.10 the only task using this framework would be the background refresh anyway, right? Then when we convert the other tasks (note: file tickets to convert the other tasks) we'll make sure that the behaviour in faulty cases stays the same so that we don't regress.
Sorry, just one more thing, is ERR_SROP_PERIODIC_TASK meant to be used to also 'gracefully' terminate a task ?
Yes.
Or only in case of fatal error.
If only in case of fatal error I wonder if using a generic error code name that can be reused in other function wouldn't be more sensible.
Maybe we can provide both?
ERR_SROP_PERIODIC_TASK for graceful termination, ERR_FATAL for unexpected termination?
Simo.