URL:
https://github.com/SSSD/sssd/pull/289
Title: #289: Check the timestamp cache during the users/groups cleanup
fidencio commented:
"""
On Wed, May 31, 2017 at 8:55 AM, Jakub Hrozek <notifications(a)github.com>
wrote:
On Tue, May 30, 2017 at 01:12:14PM -0700, fidencio wrote:
> On Tue, May 30, 2017 at 4:44 PM, Jakub Hrozek <notifications(a)github.com>
> wrote:
>
> > So the primary question I have is why not do what you're doing in the
> > ldap_cleanup module in the sysdb search?
> >
> The main answer for this question is to avoid touching code that could
> affect even more parts of the code (some of them which I'm not even aware
> on how to test that those changes are not too intrusive).
>
Yes, that's a fair point. If you prefer, we can apply less intrusive
patches first, but work on the 'prettier' solution in the meantime.
I don't think there's a need for this. Let me take advantage of your
guidance and go for the prettier solution.
> > I checked where are the sysdb_search_users and sysdb_search_groups
> > functions called and unfortunately some are in the critical paths
which are
> > already quite slow, so adding the timestamp search before persistent
cache
> > search might slow them even further..but then again, it might be
cleaner to
> > add a sysdb_search_by_orig_dn which would always search only the
persistent
> > cache.
> >
> Sorry Jakub, I'm not sure if I'm following you here. What would
> sysdb_search_by_orig_dn() do? Would it do exactly what I'm doing in the
> "LDAP_ID_CLEANUP: Check the ts_cache during the users/groups cleanup"
patch
> and just be used from ldap_cleanup()?
Yeah, sorry, that was unclear, it wasn't an explanation, but more of a
brain dump /o\ let me try again.
So your patches essentially expanded on
sysdb_search_users/sysdb_search_groups. I was checking yesterday how is
sysdb_search_{users/groups} used today and whether it would be acceptable
to just do what you did in the ldap_id_cleanup.c module inside the search
functions themselves or if the extra search in the timestamp cache might
slow some operations down.
And I noticed that almost all uses of these two search functions except
the cleanup module are searches by the original DN. At the same time,
these searches are done to find group members, so I would prefer to
keep them as fast as possible, because those codepaths are known to be a
bottleneck already.
Therefore the proposal was to add a sysdb_search_by_orig_dn, replace the
search_groups/search_users which search by DN with sysdb_search_by_orig_dn
and make the two search functions look up in the timestamp cache as well.
This would keep the interface from exposing the search-in-ts-cache
family of functions, while avoid regressing performance.
Does this make better sense?
Okay, yes, it does make sense.
As I said, I think you have a point with making minimal changes per
patch. So I wonder if a good approach would be to add additional patch
atop this set (it doesn't matter if it's a part of this PR or additional
as long as it's done eventually) that implements the by-orig-dn search.
The upstream would accept both and downstream can cherry-pick the
minimal changes.
By what you've described the proper work, although more intrusive, doesn't
look very complicated.
Let me go for the approach you suggested and re-submit thisPR.
Thanks for the suggestions,
>
> There are two reasons I propose this:
> >
> > 1. the sysdb_search_users/sysdb_search_groups interface is generic and
> > the caller should expect to return correct results regardless of
whether
> > the filter contains a timestamp attribute or not
> > 2. I don't think the sysdb.h interface should expose the timestamp
> > cache at all.
> >
> >
> We don't disagree here, with any of the points. Just keep in mind that
> sysdb_search_users() and sysdb_search_groups() do return correct results
> based on the timestamp they have. The problem here is how often we update
> the timestamp in the persistent cache.
Yes, I'm trying to think about the sysdb layer as a library. If I was a
library user of sysdb and didn't know its internals, it would be
suprising if a generic search function behaved differently depending on
what attributes you search with.
>
>
> >
> >
> > I agree that adding the search by orig dn is also a bit of a hack,
because
> > we already have too many search functions, but it seems like a more
correct
> > solution.
> >
> > —
> > You are receiving this because you authored the thread.
> > Reply to this email directly, view it on GitHub
> > <
https://github.com/SSSD/sssd/pull/289#issuecomment-304901159>, or
mute
> > the thread
> > <
https://github.com/notifications/unsubscribe-auth/AAG4et-ew
4vivfmrzG6N8ziQF7GxDlekks5r_CtngaJpZM4NpESe>
> > .
> >
>
>
> --
> You are receiving this because you commented.
> Reply to this email directly or view it on GitHub:
>
https://github.com/SSSD/sssd/pull/289#issuecomment-304994051
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<
https://github.com/SSSD/sssd/pull/289#issuecomment-305101630>, or mute
the thread
<
https://github.com/notifications/unsubscribe-auth/AAG4ejbIaGWAHlMsTpCHNhI...
.
"""
See the full comment at
https://github.com/SSSD/sssd/pull/289#issuecomment-305146818