URL: https://github.com/SSSD/sssd/pull/999 Title: #999: Mem-cache fixes/improvements
alexey-tikhonov commented: """ Hi Sumit, Thank you for the review. > in general I agree with the changes but I think the mem-cache size is not a > good config option. The concept of slots is specific to how the cache is > currently implemented Right, this doesn't seem very neat. Initially I offered Michal (this specific patch is his) to configure size in megabytes, but he convinced me that from user perspective those are the same (more on this later). > For the other `/ 8` I would suggest to round up the data from the config file > to the next integer which can be devided by 8 to be on the safe side. I think this was already implemented in `sss_mmap_cache_init()`: https://github.com/SSSD/sssd/blob/3c10595659698c7d8976bcbed88b3696c222d86e/src/responder/nss/nsssrv_mmap_cache.c#L1305 Please let me know if you see any reason to move (or duplicate?) this protection or you think it is done wrong. > I think the number of expected objects might be a better option. ... > If we have a tool to get statistics from the memory cache and this tool can > show the average payload (i.e. how many slot a cached entry is using) we > might consider to make the payload configurable as well, e.g. by enhancing > the number of expected option so that the value can be > `number_of_expected_objects:payload`. But should should be added alter. I understand your intention but IMO from user point of view this is unjustified over-complication. And most probably will not work well "in the field"... My reasoning is as follows: There is just no such thing as "average payload". What I typically meet in my experience, is that, for example, groups mem-cache may contain entries with members amount ranging from 1 to 30..60k(!) (this is typical for integration with very large AD - exactly the case when mem-cache is very important). Of course we can calculate average (or weighted average) size but this doesn't make any sense. It would rather do harm because space will be wasted for small entries and estimation will be wrong anyway. So while I understand your idea - "user knows size of own database in amount of entries => let them configure size of the cache in amount entries" - I don't think this is realistic approach. What I offer instead is perhaps very blunt but IMO should work well: "user got message in system log that mem-cache is overfill => increase size of the cache". This is less than ideal solution and still needs improvements (we should report cache overfill only if we have to erase *non expired* entries) but good in the sense of effort/result ration from my point of view (I mostly mean users here, not developers) From developers perspective, I would rather go in totally different way as opposite to "average payload" (btw, justification of those factors in the comments in the source code is very loose). I would prefer to get rid of fixed slot size completly (I must admit I didn't investigate performance impact yet but my gut feeling is it should be ok). But this would be quite large refactoring and intention of this PR - to resolve specific bug and to provide at least basic means to ajust cache for NSS load... I hope this makes sense. Please let me know what you think. """ See the full comment at https://github.com/SSSD/sssd/pull/999#issuecomment-598694168
_______________________________________________ sssd-devel mailing list -- [email protected] To unsubscribe send an email to [email protected] Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/[email protected]
