Caching #246
Caching #246
Conversation
Codecov Report
@@ Coverage Diff @@
## master #246 +/- ##
==========================================
+ Coverage 96.07% 96.86% +0.79%
==========================================
Files 133 143 +10
Lines 2496 2680 +184
==========================================
+ Hits 2398 2596 +198
+ Misses 98 84 -14
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Just some minor comments.
Will the cache automatically be updated when a plio is updated? because if not, a GET after an update will return the old value, no?
Same for deletion. When a plio is deleted, will cache automatically delete it as well? Maybe Django handles all this automatically, just curious :)
|
|
||
| @receiver([post_save, post_delete], sender=User) | ||
| def user_update_cache(sender, instance, created, raw, **kwargs): | ||
| invalidate_cache_for_instance(instance) |
There was a problem hiding this comment.
since a session also returns the user data completely, ideally, this should invalidate the cache for session instances too. however, we are not caching sessions right now as we are not making a request for sessions. but if we end up doing that in the future, is there a better way to remember that we need to invalidate its cache (when we add support for session caches) rather than just relying on our ability to recollect this then?
There was a problem hiding this comment.
But caching sessions doesn't make sense right?
I don't think we ever make a GET for session right?
Every time someone comes to a plio, we create a new session
There was a problem hiding this comment.
When we end up adding more details to the dashboard, we'll be fetching the sessions then na?
|
|
||
| @receiver([post_save, post_delete], sender=OrganizationUser) | ||
| def organization_user_update_cache(sender, instance, **kwargs): | ||
| invalidate_cache_for_instance(instance.user) |
There was a problem hiding this comment.
similar comment as above - is there a better way to be sure that we won't forget updating the relevant cache instances for whom we've not added caching support right now (and all the other objects connected to them)? Ideally, there could be like a graph of objects, each connected to the ones which it relies on (based on a FOREIGN_KEY relation) and the one which have been cached are marked in, say, green, with the remaining ones marked in, say, red. Is there a tool that can do this mapping for us?
There was a problem hiding this comment.
I think probably that's why caching is one of the most difficult things to implement :)
Yes, I agree, we need some sort of UI representation to easy understand connected tables. An ER diagram will help definitely. I'll share it.
Co-authored-by: Aman Dalmia <[email protected]>
|
|
||
| @receiver([post_save, post_delete], sender=User) | ||
| def user_update_cache(sender, instance, created, raw, **kwargs): | ||
| invalidate_cache_for_instance(instance) |
There was a problem hiding this comment.
But caching sessions doesn't make sense right?
I don't think we ever make a GET for session right?
Every time someone comes to a plio, we create a new session
Co-authored-by: Aman Dalmia <[email protected]>
Fixes #235, #224
Summary
Caching Outcome
Screenshot from local docker
Right: Plio with 12 items and querying Postgres DB (no cache)
Left: Same Plio with 12 items and querying Redis (cached)
Test Plan