On Thu, Jun 23, 2016 at 09:52:46AM +0200, Jakub Hrozek wrote:
> On Tue, Jun 21, 2016 at 10:28:52PM +0200, Lukas Slebodnik wrote:
> > On (21/06/16 22:08), Jakub Hrozek wrote:
> > >On Tue, Jun 21, 2016 at 11:46:51AM +0200, Jakub Hrozek wrote:
> > >> On Mon, Jun 20, 2016 at 06:57:55PM +0200, Jakub Hrozek wrote:
> > >> > On Fri, Jun 10, 2016 at 01:33:49PM +0200, Jakub Hrozek wrote:
> > >> > > On Fri, Jun 03, 2016 at 12:43:41PM +0200, Jakub Hrozek
wrote:
> > >> > > > Hi,
> > >> > > >
> > >> > > > the attached patchset improves SSSD by performance by
adding a second
> > >> > > > ldb cache file that is opened with LDB_FLG_NOSYNC and
contains empeheral
> > >> > > > data like timestamps. The reason is to avoid updating
the sync-cache in
> > >> > > > case nothing in fact changed and SSSD is only
refreshing the cache to
> > >> > > > add new timestamps. Especially for group lookups, this
can improve
> > >> > > > performance a bit. The first lookup, or any where the
actual data
> > >> > > > changes is still slow, though.
> > >> > > >
> > >> > > > I don't have a CI link yet, because I'm having
issues installing pyldb on
> > >> > > > our test machines.
> > >> > > >
> > >> > > > The last patch doesn't touch sysdb, but the LDAP
provider. I hope it's
> > >> > > > fine to include it in the same set, though.
> > >> > > >
> > >> > > > The full design document is at:
> > >> > > >
https://fedorahosted.org/sssd/wiki/DesignDocs/OneFourteenPerformanceImpro...
> > >> > >
> > >> > > the attached patches fix one bug in the NSS responder
related to MPG
> > >> > > domains and one issue with adding incomplete groups.
> > >> >
> > >> > I found another bug:
> > >> > if you upgrade to this version from one that didn't have
the
> > >> > timestamp cache, sss_cache doesn't work until the cache
is
> > >> > invalidated.
> > >> >
> > >> > I'm not sure how to fix this yet, whether to create the
timestamp
> > >> > entries on upgrade to be safe or maybe invalidate all entries
during
> > >> > upgrade..but I would like to wait if there are any review
comments from
> > >> > anybody before proceeding.
> > >>
> > >> Attached are patches rebased on today's origin/master.
> > >
> > >btw the integration test I wrote seems a bit flaky in CI:
> > >
http://sssd-ci.duckdns.org/logs/job/45/68/rhel6/ci.html
> > >although it just works for me locally. I wonder if it would be
> > >acceptable to use the test only during patch review and fix it up later.
> >
> > You needn't remove test from patch.
> > Just remove file with test.
> > It should not start with "test_" or finish with "_test.py"
>
> sleep(1) after the modification and before requesting the entry "solved"
> it.
>
> The attached patches also fix several Coverity issues and other review
> comments Sumit found during his review.
>
> CI:
http://sssd-ci.duckdns.org/logs/job/45/80/summary.html
Thank you for the patches. In my tests I've seen 5-10x faster lookups of
expired cached objects which didn't change on the server.
All my comments I discussed with Jakub on irc are included in the latest
patch set and CI in general passed as well
http://sssd-ci.duckdns.org/logs/job/45/81/rhel7/ci.html (the failure on
RHEL7 is in test_add_remove_membership_rfc2307_bis where I've seen
similar issues with other runs, maybe it is a timing issue as well, i.e.
we re-read the data from the server before 389ds was able to properly
update the related object?).
ACK
bye,
Sumit
Thank you very much for the review. I pushed all the patches to master:
* beec1ee5799570f34a51ea57674c7291c15f7022
* b8946a5dbde01a87465de707092716349a35248b
* d36f4db9bb5efc63b94190cca25adb08ee56971c
* 3bd9da80f71a6794af0a6b3fbc11bc3a2da64638
* 4016c7dd288d379118b47ecbe7d8f46cfcb0d400
* 40de79d69860ec7f04bf7795bd88b641ec42fd23
* a257259b05d62ebe548b6c798a3aa03a97dbc0c2
* dd285415d7a8d8376207960cfa3e977524c3b98c
* 13d7df10bf4d76c333a9169f9fcbeb891d870351
* f983b400bf4f6fb14a2174d6f58071e06e9ec832
* f21b3cce14055e77af8ccb98dd8e0fa1ec1f7944
* 72dbcd0a3361f1c0f0c3e348aa2fbcabd926188b
* e732d23f3ec986a463d757781a334040e03d1f59
* 6e9d7cbe43fdfc866b18f9ef0779bbfc10ad6f3a
On IRC we talked about adding more debugging to the code. Because I'm sure
I would forgot otherwise, I filed:
- Add systemtap probes into
the top-level data provider requests
The latter would be useful in case some users don't see the promised
performance enhancement, at least it would allow us to see which requests
in general take so long. We already have probes for LDAP provider, but
not the others.