[SSSD] [PATCH] The new memberof plugin

Stephen Gallagher sgallagh at redhat.com
Thu Sep 15 15:37:12 UTC 2011


On Thu, 2011-09-15 at 17:29 +0200, Jan Zelený wrote:
> > On Thu, 2011-09-15 at 17:00 +0200, Jan Zelený wrote:
> > > > On Tue, 2011-08-30 at 17:54 +0200, Jan Zelený wrote:
> > > > > Attached patch rewrites almost entire memberof plugin. It heavily
> > > > > utilizes hash tables instead of lists and arrays and it introduces
> > > > > concept of reference counting which should heavily optimize all
> > > > > operations when no loops are present in the user/group tree.
> > > > > 
> > > > > I tested the patch by running sysdb test suite, all tests passed. I
> > > > > also attach a document where basic concepts of the plugin are
> > > > > explained.
> > > > > 
> > > > > Please note that the patch is functional, although I don't consider
> > > > > it ready. I just want to get pre-ACK or some comments about the
> > > > > patch design. I have yet to implement the recompute task, I will
> > > > > work on that later.
> > > > 
> > > > Jan,
> > > > yesterday I an Stephen got together to start a review of the patch. Due
> > > > to the complexity we tried to go thorugh the code together to make sure
> > > > we understand what is going on.
> > > > 
> > > > Also due to the complexity and need to refresh memory we were only be
> > > > able to go through most of the "add" code only, we were not able to
> > > > review the update or delete code paths yet.
> > > > 
> > > > Attached find a diff file with comments in C++ style (so that it is
> > > > clear they all need to be resolved before the patch can be considered
> > > > ok, as they show as a big red line in our editors) that you can
> > > > integrate in your tree and check one by one.
> > > > 
> > > > There are some questions in there, but in general we noticed a very
> > > > annoying lack of comments in the code itself. The accompaning PDF is
> > > > nice and all, but unless the logic is embedded and explained in the
> > > > code (like it was with the previous code) it will be ablocker and will
> > > > make review more difficult.
> > > > 
> > > > That said, so far so good, although we need to more carefully review
> > > > the refcounting stuff, we got to it late and only marginally reviewed
> > > > it. Need more time to fully grasp all details.
> > > > 
> > > > HTH,
> > > > Simo.
> > > 
> > > Simo,
> > > first I'd like to thank you both for the review. I realize it is quite
> > > difficult, I wouldn't want to do it myself.
> > > 
> > > I will look at the diff you wrote about ASAP. That said, I already have a
> > > modification which resolves some pretty big issues which I somehow
> > > managed to miss at first. It also modifies the full update code path so
> > > it can be used by the recompute task. I will certainly add some more
> > > comments, up until now I've been focusing on the code itself. Hopefully
> > > I'll have the patch complete by the end of the week (i.e. tomorrow) and
> > > I'll send it. If you wish, we can schedule a phone call to discuss some
> > > places which are the most difficult to comprehend.
> > 
> > Yes we should set up a call between me you and Stephen next week. so we
> > can start a through review.
> > 
> > Also one other thing we forgot to mention is that we were wondering if
> > you made any performance measurements.
> > 
> > Perf. was the main driver for this work, so we would like to see some
> > numbers to make sure we did actually achieved perf. gains with the new
> > patchset. It would be pretty lame to switch a very complex system with
> > another very complex (and largely untested) system and find out we got
> > little to no gain :-)
> > 
> > Simo.
> 
> I did some testing and I was disappointed, the improvement was very small. But 
> on the other hand, I was just running the sysdb test suite and the main 
> difference should be seen when using complex rfc2307bis membership structures. 
> I will set up a testing environment during the next week, hopefully I'll get 
> some satisfying results.

The sysdb test suite isn't a good example. (Though it would be best if
you could expand it to do so).

The biggest performance gains SHOULD be when removing a member attribute
from a complicated relationship. Please measure this and let us know.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110915/ec72170e/attachment.sig>


More information about the sssd-devel mailing list