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/3c10595659698c7d8976bcbed88b3696c222d86...
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