Hi,
On 05/21/2013 12:06 PM, Jakub Hrozek wrote:
On Wed, May 15, 2013 at 11:45:47AM +0200, Pavel Březina wrote:
I have a couple of questions.
[PATCH 1/4] back end: periodic task API
What is the purpose of BE_PTASK_OFFLINE_SKIP? Why not simply let the
back end go online and re-enable the task instead?
I'm sorry, I don't follow.
The purpose of it is: If we are about to execute request and back end is
offline, we just skip current execution and reschedule it to now +
period. It is current behaviour of enumeration and other tasks (I'm not
sure about dyndns).
I think you should either always store last_execution time or introduce
last_attempt. The reason is that if back end was offline for a long
time and then be_ptask_schedule was called with
BE_PTASK_SCHEDULE_FROM_LAST then the task might be scheduled in the
This value is only internal. Do you see any code path when this is
possible? If so then I think it is a bug.
past. I'm not sure how tevent behaves in this respect.
Tevent would just trigger the timer in the next opportunity. It's the
same as if you create timer to T and enter the event loop in T+x.
In general it's not clear to me from the code who is responsible
for
freeing the task -- is it the caller, but only on success? I think this
should always behave the same.
I'm sorry, I don't know what you mean.
be_ptask_schedule(): The DEBUG message uses %d but passes in a string.
Thanks, will fix that.
It's not even d but đ (alt+s), a relic from using czech keybord (I know,
I'm weird, but I never got used to anything else :-))
[PATCH 2/4] back end: periodical refresh of expired records API
The "enum be_refresh" name is too generic. Maybe "enum
be_refresh_type"
?
In be_refresh_get_names(), can you use sysdb_attrs_to_list() to gather
the names?
Is it wise to assume that all object have names and name the getter
get_names()? Some object might not have names at all, but for instance
UUIDs..
I thought every sysdb object has a name. What objects don't?
I'll rename it then to something more generic.
Also in be_refresh_step() you call get_names but get back a
"dn".
Sorry, about that. I wanted to use originalDN but I was forced to change
it to name, because netgroups API uses name.
Please file tickets to add unit tests for these two modules and make the
tickets block upstream #1923.
Nitpick:
> + filter = talloc_asprintf(tmp_ctx, "(&(%s<=%lld))",
> + SYSDB_CACHE_EXPIRE, (long long)now);
^^^
missing space
[PATCH 3/4] back end: add refresh expired records periodic task
> --- a/src/config/SSSDConfig/__init__.py.in
> +++ b/src/config/SSSDConfig/__init__.py.in
> @@ -125,6 +125,7 @@ option_strings = {
> 'entry_cache_service_timeout' : _('Entry cache timeout length
(seconds)'),
> 'entry_cache_autofs_timeout' : _('Entry cache timeout length
(seconds)'),
> 'entry_cache_sudo_timeout' : _('Entry cache timeout length
(seconds)'),
> + 'refresh_expired_interval' : _('How often should expired rules be
refreshed in background'),
We should use "objects" or "entries" instead of rules.
Good catch, I used sudo option as copy source :-)
Can you explain the "circular dependency" here? I think you
already told
me in person, but I forgot..
Let me answer with question - do you see a way how to easily remove it
in scope of 1.10? If yes, I'm all for it. But I think we should not mess
with back end and provider code at this point.
> --- a/src/providers/dp_ptask.h
> +++ b/src/providers/dp_ptask.h
> @@ -27,6 +27,9 @@
>
> #include "providers/dp_backend.h"
>
> +/* solve circular dependency */
> +struct be_ctx;
> +
> struct be_ptask;
>
> /**
> diff --git a/src/providers/dp_refresh.h b/src/providers/dp_refresh.h
> index
54c6aac99866415d4f33f61ac05d487f71b8c49c..d93a932637804dfbf743c395af3f8e640f79c12f 100644
> --- a/src/providers/dp_refresh.h
> +++ b/src/providers/dp_refresh.h
> @@ -27,6 +27,9 @@
> #include "providers/dp_backend.h"
> #include "providers/dp_ptask.h"
>
> +/* solve circular dependency */
> +struct be_ctx;
> +
[PATCH 4/4] providers: refresh expired netgroups
I don't see one important part implemented -- what if all netgroups
expire at once, then they are all refreshed at once, right? Can we add
some throttling and refresh them in batches with some delay to avoid
starving the back end?
I thought we agreed in person that we will first push a basic stupid
solution and than fine tune it if necessary - hopefully based on real
environment.