On 07/29/2014 03:29 PM, Jakub Hrozek wrote:
>
> So far I only read the patches, no testing yet, see some comments
> inline:
>
>> From 0724c54b3e4ff73b091afed86122cdf8dbc746ea Mon Sep 17 00:00:00 2001
>> From: Michal Zidek <mzidek(a)redhat.com>
>> Date: Mon, 7 Jul 2014 16:46:18 +0200
>> Subject: [PATCH 1/3] ptask: Allow adding random_offset to scheduled
>> execution
>> time
>
> ACK
>
>> From bf9d6aebda21397278b5a3f782efaa1a538992cb Mon Sep 17 00:00:00 2001
>> From: Michal Zidek <mzidek(a)redhat.com>
>> Date: Mon, 7 Jul 2014 17:45:45 +0200
>> Subject: [PATCH 2/3] ptask: Add backoff feature to the ptask api.
>
> I would prefer if the backoff was a parameter to the create function,
> creating would better be atomic.
Ok.
>
>> From fd85d23e9721293adaaaba9927947d31ab68da8a Mon Sep 17 00:00:00 2001
>> From: Michal Zidek <mzidek(a)redhat.com>
>> Date: Thu, 3 Jul 2014 12:49:20 +0200
>> Subject: [PATCH 3/3] Exit offline mode only if server is available.
>>
>> This patch adds periodic check to test if
>> backend can exit offline mode and only
>> marks backend as not offline if server
>> for the service is available.
>>
>> Prior to this patch we marked backend as not
>> offline if the offline_timeout was reached
>> without checking for the server availability
>> and when the next request failed again
>> we switched back to the offline mode.
>> This caused significant slowdowns in some
>> edge cases.
>>
>> Fixes:
>>
https://fedorahosted.org/sssd/ticket/2355
>> ---
>> src/providers/data_provider_be.c | 68
>> ++++++++++++++++++++++++++++++----------
>> src/providers/dp_backend.h | 5 +++
>> 2 files changed, 56 insertions(+), 17 deletions(-)
>>
>> diff --git a/src/providers/data_provider_be.c
>> b/src/providers/data_provider_be.c
>> index 4c3eaa6..76b7f82 100644
>> --- a/src/providers/data_provider_be.c
>> +++ b/src/providers/data_provider_be.c
>> @@ -450,35 +450,68 @@ static void be_queue_next_request(struct be_req
>> *be_req, enum bet_type type)
>>
>> bool be_is_offline(struct be_ctx *ctx)
>> {
>> - time_t now = time(NULL);
>> - int offline_timeout;
>> - int ret;
>> + return ctx->offstat.offline;
>> +}
>>
>> - /* check if we are past the offline blackout timeout */
>> - ret = confdb_get_int(ctx->cdb, ctx->conf_path,
>> - CONFDB_DOMAIN_OFFLINE_TIMEOUT, 60,
>> - &offline_timeout);
>> - if (ret != EOK) {
>> - DEBUG(SSSDBG_MINOR_FAILURE,
>> - "Failed to get offline_timeout from confdb. "
>> - "Using default value (60 seconds)\n");
>> - offline_timeout = 60;
>> - }
>> +static void check_if_online(struct be_ctx *ctx);
>>
>> - if (ctx->offstat.went_offline + offline_timeout < now) {
>> - ctx->offstat.offline = false;
>> - }
>> +errno_t
>> +try_to_go_online(TALLOC_CTX *mem_ctx,
>> + struct tevent_context *ev,
>> + struct be_ctx *be_ctx,
>> + struct be_ptask *be_ptask,
>> + void *be_ctx_void)
>> +{
>
> Can you make this function static?
Done.
>
>> + struct be_ctx *ctx = (struct be_ctx*) be_ctx_void;
>>
>> - return ctx->offstat.offline;
>> + check_if_online(ctx);
>> + return EOK;
>> }
>>
>> void be_mark_offline(struct be_ctx *ctx)
>> {
>> + int offline_timeout;
>> DEBUG(SSSDBG_TRACE_INTERNAL, "Going offline!\n");
>>
>> ctx->offstat.went_offline = time(NULL);
>> ctx->offstat.offline = true;
>> ctx->run_online_cb = true;
>> + errno_t ret;
>> +
>> + if (ctx->online_check_ptask_initialized == false) {
>
> Can you use just:
> if (ctx->check_if_online_ptask == NULL)
>
> ?
>
> If not, then instead of another structure member, isn't it more readable
> to add a new function be_ptask_is_enabled() and then you could call:
> if (ctx->check_if_online_ptask &&
> be_ptask_is_enabled(ctx->check_if_online_ptask)) {
> }
>
> The andvantage is that you don't have to keep track of anything
> separately and all the state info is kept in the ptask itself.
Ok. I'll go with the NULL check then.
New patches are attached.
Michal
Jakub found one more coding style violation in the code.
Fixed in the new patches.
Thanks,
Michal