Initial version of Caching Rewrite RFC #615
Conversation
✅ Docs preview has no changesThe preview was not built because there were no changes. Build ID: 666fa45e078155674b74a69b |
✅ Deploy Preview for apollo-ios-docc canceled.
|
There was a problem hiding this comment.
This is a great RFC to kick-off the discussion! Thanks @BobaFetters!
One thing that I think is missing from this is a discussion of changes to the NormalizedCache protocol's API itself. I know that we have gotten requests from users to give the cache protocol functions access to more context of the operation that is being run during a cache read (looking at you @jimisaacs 😉).
I'd like to get some more feedback from users on this RFC about what would be needed there and see if we want to include changes to that protocol in this update as well.
|
|
||
| ### Directive | ||
|
|
||
| We will create a new local directive `@cacheControl(maxAge: Int)`, where the `maxAge` is measured in seconds, which can be applied to objects and fields in your schema or operations: |
There was a problem hiding this comment.
I don’t love having to handle maxAge in seconds always. It’s one thing to do the math once when you’re setting it up, but every time you are going back to refer to it, it’s frustrating to have the mental overhead of trying to figure out that, for example 604800 means one week. This could be a good place to use a @OneOf input object here that had fields for seconds, minutes, and days.
I know we are trying to match the cacheControl directive used by Apollo Kotlin. Is there a chance that both teams can align on a new version of this directive API?
There was a problem hiding this comment.
We have an internal meeting about this scheduled for tomorrow to get alignment between client teams can discuss then.
| public struct NormalizedCacheConfiguration { | ||
| let sizeLimit: Int | ||
| let autoEvictionSize: Int | ||
| let evictionFieldsIgnoreList: [Field] |
There was a problem hiding this comment.
What is the API of this Field type? How is this going to reference a specific field on a specific entity type? What about referencing a field on an interface type?
There was a problem hiding this comment.
Initially thinking it would be Field although looking through this again, makes more sense to probably have an Object or Interface and Field together as a tuple so you could specify fields on an object or interface and the field type has the extra context from the object/interface as well.
|
|
||
| ### Cache Size Limits | ||
|
|
||
| Configuring cache size limits will be done using the `sizeLimit` property of the `NormalizedCacheConfiguration` and will represent the maximum size in kilobytes (KB) you wish the cache to be before evicting some data to free up space. |
There was a problem hiding this comment.
I know I had originally said we should change this from MBs to KBs, but I'm thinking that the easiest API to use might be MBs as a decimal number. You don't want MBs as an Int b/c then you can only configure it in increments of a MB. But with a decimal number you could configure it more granularly (eg. 3.25 MBs).
There was a problem hiding this comment.
I'm open to making this MB again, technically if we want to more closely match other caching apis such as NSCache etc they typically work off of bytes which may be more familiar to people when working with this. Would love to hear thoughts from the community though.
|
|
||
| By default the automatic eviction done from the caches will be handled as a least recently used (LRU) style cache. However, by using the `evictionFieldsIgnoreList` property on the `NormalizedCacheConfiguration` you will be able to provide an array of `Field` objects representing fields from any of your types you wish to not have evicted for any reason, such as it being long lived data that is unlikely to change. | ||
|
|
||
| ### Cache Overflow Handling |
There was a problem hiding this comment.
I think we need to put more thought into the Cache Overflow handling. Nothing here is wrong, it just feels incomplete to me. The complexity of how users can do manual cache eviction are unclear to me right now and what common use cases for that are. I’m also not sure if 20% of the cache size limit is always going to be desirable for auto eviction. Might need more configuration of that.
There was a problem hiding this comment.
Can add more detail for manual cache eviction, should support either single object deletion, transaction for group deletions of multiple objects, or full cache clear as a base.
Also the amount that gets clear from auto eviction is already detailed as being configurable here.
|
|
||
| ### Other options | ||
|
|
||
| We have considered an implementation of cascading deletions where a heuristic is used to determine relationships between objects, which would detect parent/child object relationships and having the default behavior be to always delete child objects with parent objects. Currently we don't plan to move forward with this implementation. |
There was a problem hiding this comment.
Thanks for adding the mention of this discussion. After consideration, I agree that this isn't probably something worth implementing, but I wanted a note here so that others can give us feedback on this.
| private let sqliteCache: SQLiteNormalizedCache | ||
|
|
||
| private let inMemoryExcludeList: [Object] | ||
| private let sqliteExcludeList: [Object] |
There was a problem hiding this comment.
The exclude lists could easily put the cache in a state where you are inadvertently getting a lot of cache misses that you don’t want. Maybe there is a better way to handle this that would be more user-friendly. I think we need to first answer the question of what use-cases would anyone actually want to use this for, then we can design the right APIs to enable the use cases without making it a massive foot-gun.
I have a feeling that it would make sense to have the ability to exclude short-lived data from being cached in the persistent SQLite cache, but I can't come up with a situation in which you would want to exclude something from the in-memory cache but still have it in the SQLite cache. That would just lead to a lot of repeated cache misses.
There was a problem hiding this comment.
Main use case I can think of is preventing short lived data between app runs by excluding from the sqlite cache. No specific use case for excluding from in-memory cache, although I guess it would allow an in-between of no caching and only caching some data in memory.
I don't think this would put the cache in a state where you get inadvertent cache misses because users would be explicitly saying what they dont want in the caches therefor it should result in a cache miss. Imagine feedback from the community here about how they may use this feature would go a long way.
| private let inMemoryCache: InMemoryNormalizedCache | ||
| private let sqliteCache: SQLiteNormalizedCache |
There was a problem hiding this comment.
I know we had discussed having the ability to provide an array of [any NormalizedCache] objects to be chained together, but that does seem like it's adding complexity for the sake of flexibility that I don't think is actually ever needed. So I am on board with only allowing you to provide a single in-memory cache and a single SQLite cache.
However, users currently have the ability to provide their own NormalizedCache conforming types, and I think that it makes sense that they would want to provide those to this as well. Maybe there should be some way of denoting the semantic difference between an in-memory and persistent cache, but I still think we should allow users to use those with chained caching.
There was a problem hiding this comment.
With us providing the chained cache as an implementation of NormalizedCache I think it makes sense that it accepts the 2 cache implementations we provide. Still leaves open the possibility for users to create their own chained caches to go along with custom caches. If we want to support this we could added protocols for in-memory vs persistent caches that inherit from normalize cache and go that route. Would need to consider if theres any added functionality the protocols would need to properly support the chaining or if it would mostly just be plug and play. Curious if thats some the community would find useful to have.
Waiting to add anything about the |
Read the formatted RFC here