Stop updating client cache when revalidated and still expired #549
Conversation
There was a problem hiding this comment.
For VBase requests, this implementation makes the remove of await from storage.set unnecessary. Should we merge the changes on PR and test it all together? We are trying to create a test env to evaluate the changes before going live with it. WDYT?
Do you mean this test environment? I like the idea of having that but it seems unnecessary for this change, since I was able to test it very well by just linking, with console.log + tracing. What's the main purpose of using this new test environment? What kind of changes would require using it? As for merging with the PR for running the saves on the background, I feel like it's better to deploy them separately. This one is ready and it has some extra logic related to checking expiration. It would already help VBase's case, like you said. It would be better to observe it on its own before deploying writes on the background, since those will affect other clients besides VBase. These 2 PRs affect the apps in different ways, so the things we need to test for each are different as well. |
|
Not necessary that one, but any test env that allow us a |
What is the purpose of this pull request?
This is removing a bunch of unnecessary disk writes, which just waste CPU.
What problem is this solving?
When a cached response is expired, we try sending a request using the cached etag to revalidate it. If this revalidation is successful, we use the cache contents. In that case, we also update the cache again with the contents it already has. That's just a waste of CPU.
This is only ever useful if the cache max age has increased, to avoid further revalidations, since we also write the new max age to the disk. But at least for pages-graphql calls it's very frequent for the content to still be expired at this point.
This PR is making sure that we only ever update the cache if either the content has changed (meaning that we didn't use the cached contents) or the expiration time has changed.
This should free a lot of disk writes from pages-graphql (at least ~60% of disk writes for VBase calls, plus others in the same situation).
How should this be manually tested?
I've tested this by vtex linking pages-graphql with @vtex/api locally linked, and adding console.log to show when we skip writing to cache. The skips were done at the right time (after revalidating VBase requests, which are always expired).
I've also tested by looking at Honeycomb traces. On production you'll always see the
local-cache-savedevent for requests to VBase, even for STALE requests. Now you'll only ever see this event for MISS requests to VBase (example trace with STALE, example trace with MISS).Screenshots or example usage
Types of changes