-
Notifications
You must be signed in to change notification settings - Fork 87
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ACL Cache #883
base: master
Are you sure you want to change the base?
ACL Cache #883
Conversation
I found a Bug that the cache is not cleared correctly sometimes that i still need to fix... So this should not be merged yet. |
|
||
public function __construct(ICacheFactory $cacheFactory, IUserMappingManager $userMappingManager) { | ||
$this->userMappingManager = $userMappingManager; | ||
$this->ruleCache = $cacheFactory->createDistributed('acl'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$this->ruleCache = $cacheFactory->createDistributed('acl'); | |
$this->ruleCache = $cacheFactory->createDistributed('groupfolders-acl'); |
Maybe a bit safer in terms of naming collisions.
There might also be an issue with the cache invalidation if paths change e.g. through renaming with this approach, so it might be useful register some files hooks and invalidate then as well. |
Thanks @Deltachaos, this looks like a valuable contribution. Would you mind rebasing this to the current version, fix the remaining bugs and incorporate a solution for invalidating the cache on renaming events as proposed by @juliushaertl? |
I am currently out of time to do this. Maybe this is something @JonathanTreffler from @verdigado can work on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would also be great to have some explanation in a commit about how the data in the cache is stored/structured
$ttl = self::DEFAULT_TTL; | ||
} | ||
|
||
$ttl = rand($ttl, round($ttl * self::DERIVATION)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks wrong, this currently select a ttl between 0.1 days and 1 days, while I would expect a derivation of 0.1 to mean something like 0.9 days to 1 day
I also wonder if this should include some test coverage to make sure the caching and invalidation of the cache will continue to work properly in the future? |
@JonathanTreffler Do you or anyone else still care about getting this finished? |
Am I right to assume that #1694 already implements ACL caching as proposed here, at least part of it? If yes I will close this PR. |
We have enabled the ACL feature for hundreds of group folders in a large and heavy used installation. This had tripled the load on our database server because of a large number of additional database queries where required to generate the file lists.
If you have a lot of sync clients this gets even worse. The response times of the application are also increasing by a few hundreds of milliseconds.
Because of this i have implemented a cache for the ACL manager that keeps the actively used rules in the memory. Even when stored in Redis, this increases the performance dramatically.