Skip to content
This repository has been archived by the owner on Jan 6, 2023. It is now read-only.

Implement smarter caching strategy #659

Closed
honzabilek4 opened this issue Dec 17, 2018 · 16 comments
Closed

Implement smarter caching strategy #659

honzabilek4 opened this issue Dec 17, 2018 · 16 comments
Labels
enhancement New feature or request

Comments

@honzabilek4
Copy link

honzabilek4 commented Dec 17, 2018

Currently, when caching is enabled for the api, it considerably speeds up the response times. For example, it takes only around 15-20ms for us, which is awesome.

However, the whole feature becomes unusable, because all item listings are cached for the amount of TTL. This means that you cannot see any newly created items until the TTL expires.

The usual approach to solve this problem is invalidating the cache entries when an item is being added, deleted or updated. This is would be also the easiest, because new listings will be added back to cache while being read.

I think that this feature might be important to be able to scale the entire API instance and to lower some server workload.

@honzabilek4
Copy link
Author

honzabilek4 commented Dec 17, 2018

If needed, I'll be also able to help with this one, after some quick consultation with @wellingguzman.

@rijkvanzanten
Copy link
Member

There's another side to this though, if you're updating items at a very high rate, you might not want to invalidate the cache every time.

I think there should be a function that can be called to invalidate the cache that you then can call from a hook. That way, the user is in full control of when the cache gets refreshed.

@honzabilek4
Copy link
Author

I agree, but cannot come up with any use case which would require updating items at very high rate. Do you have any example?

@rijkvanzanten
Copy link
Member

Your IoT device is pushing the sensor value every minute, you have a web app to visualize the last hour of values

@honzabilek4
Copy link
Author

honzabilek4 commented Dec 17, 2018

Thanks for the example.
Using hooks for this purpose is a good way to go. But maybe some additional cache configuration option would be easier to manage? (I see imperative vs declarative approach)

@rijkvanzanten rijkvanzanten added the enhancement New feature or request label Dec 23, 2018
@rijkvanzanten
Copy link
Member

Also, I think directus_* collections shouldn't be cached. When changing the permissions it should propagate immediately

@honzabilek4
Copy link
Author

I wouldn't even mind caching the user created collections only.

@stale
Copy link

stale bot commented Feb 21, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Feb 21, 2019
@honzabilek4
Copy link
Author

honzabilek4 commented Apr 13, 2019

Can anyone take a look at this? Or can we discuss how can we implement this feature? Without the invalidation, the cache is unusable right now. But all of us could definitely benefit from the redis (or other cache) integration. I'm personally willing to work on this, just need some discussion on how to do it exactly.

@rijkvanzanten
Copy link
Member

I think having one skip-cache flag in the API would already help out tremendously. That would allow the application to work with the API. Secondly, we should have an option where the cache gets invalidated on save instead of based on time. @BJGajjar @theharshin @hemratna

@honzabilek4
Copy link
Author

I love this Idea! 🙂😊

@theharshin
Copy link
Contributor

@honzabilek4 Does this PR helps to solve your current requirement? #892
As @rijkvanzanten suggested, we can still consider adding a skip-cache flag in endpoints to get live data.

@honzabilek4
Copy link
Author

Thanks! It seems like #892 could solve the issue. I'm going to test it in the following days and close the issue if applicable.

@benhaynes
Copy link
Member

Hey @honzabilek4 — any updates? :)

@bminer
Copy link

bminer commented Dec 6, 2019

Just curious the /me route, which displays info about currently logged in user gets cached, and it shouldn't because it depends on the auth token. Thoughts on this? Should I open a separate issue?

@bminer
Copy link

bminer commented Dec 6, 2019

^^^ Reminds me of #1476, so maybe we can continue that conversation there...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants