On 11/18/2012 06:09 PM, Jakub Hrozek wrote:
On Fri, Nov 16, 2012 at 11:41:50AM +0100, Pavel Březina wrote:
> On 11/15/2012 07:51 PM, Jakub Hrozek wrote:
>> In case a service is restarted while the DP is not ready yet, it gets
>> restarted again immediatelly, which means the DP might still not be
>> ready. The allowed number of restarts is then depleted quickly.
>>
>> This patch changes the restart mechanism such that the first restart
>> happens immediatelly, the second is scheduled after 2 second, then 4
>> etc..
>>
>>
https://fedorahosted.org/sssd/ticket/1528
>
> Hi,
>
>> + /* restarts are schedule after 0, 2, 4, ...seconds */
>> + tv = tevent_timeval_current_ofs((svc->restarts << 1), 0);
>> + te = tevent_add_timer(svc->mt_ctx->ev, svc, tv, mt_svc_restart, svc);
>
> I would consider setting 4 seconds as a maximum delay instead of
> going to infinity. Any opinions?
I agree. I wanted to make the delays self-configurable but setting an
upper limit is the right thing to do.
>
>> +static void mt_svc_restart(struct tevent_context *ev,
>> + struct tevent_timer *te,
>> + struct timeval t, void *ptr)
>> +{
>> + struct mt_svc *svc;
>> +
>> + svc = talloc_get_type(ptr, struct mt_svc);
>> + if (svc == NULL) {
>> + return;
>> + }
>> +
>> + DEBUG(SSSDBG_TRACE_FUNC,
>> + ("Scheduling a service for restart %d\n",
svc->restarts));
>
> It should say svc->restarts + 1. Can you also add a service name to the
> message?
Done and done. Thank you for the review.
Hi,
nitpicks...
+/* TODO: get the restart related values from config */
+#define MONITOR_RESTART_CNT_RESET 30
I had some troubles interpreting this macro. Can you change the name in
such way that it is more clear that we are resetting restart counter
after 30 seconds? Maybe
MONITOR_RESET_RESTART_COUNTER_INTERVAL
+ /* restarts are schedule after 0, 2, 4 seconds */
+ restart_delay = svc->restarts << 1;
+ restart_delay = (restart_delay > MONITOR_MAX_RESTART_DELAY) ? \
+ MONITOR_MAX_RESTART_DELAY : \
+ restart_delay;
I think this is easier to read:
/* restarts are schedule after 0, 2, 4 seconds */
restart_delay = svc->restarts << 1;
if (restart_delay > MONITOR_MAX_RESTART_DELAY) {
restart_delay = MONITOR_MAX_RESTART_DELAY;
}