caching: Correctly calculate cached responses age#12802
caching: Correctly calculate cached responses age#12802htuch merged 16 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
|
/retest |
|
Retrying Azure Pipelines, to retry CircleCI checks, use |
…er than an internal header Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
|
@jmarantz I think this is ready for you to take a look! |
snowp
left a comment
snowp
left a comment
There was a problem hiding this comment.
Thanks this looks pretty good to me, just a couple small comments
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
jmarantz
left a comment
jmarantz
left a comment
There was a problem hiding this comment.
basically lgtm with one suggestion to add to a TODO.
| size_t stableHashKey(const Key& key); | ||
|
|
||
| // The metadata associated with a cached response. | ||
| // TODO(yosrym93): This could be changed to a proto if a need arises. |
There was a problem hiding this comment.
for now, yes. However, it's worth noting that if a persistent cache is created using this code, and then you change it to a proto, you'd need to somehow invalidate the entire cache when referenced by the new code.
Maybe add that to the comment? Also how would we invalidate the cache? Would we put a version # into the key?
There was a problem hiding this comment.
I added this to the comment.
As for the how, IIUC, cache invalidation will need to be implemented at some point in the future anyway.
There was a problem hiding this comment.
For the scenario where we need to invalidate everything, we would alter the key generation--if there isn't anything more situation-specific, we might add a version #.
|
/wait |
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
|
@htuch This should be ready to merge now. |
|
@snowp can you merge if your feedback is now addressed? Thanks. |
| size_t stableHashKey(const Key& key); | ||
|
|
||
| // The metadata associated with a cached response. | ||
| // TODO(yosrym93): This could be changed to a proto if a need arises. |
There was a problem hiding this comment.
For the scenario where we need to invalidate everything, we would alter the key generation--if there isn't anything more situation-specific, we might add a version #.
5 participants
Commit Message:
The CacheFilter now correctly calculates the age of cached responses.
Signed-off-by: Yosry Ahmed yosryahmed@google.com
Additional Description:
The time at which responses are received is recorded, and stored with cached responses as
response_time. When a response is served from cache,response_timeand other headers are used to correctly calculate the age of the cached response.Risk Level: Low
Testing: Unit tests.
Docs Changes: N/A
Release Notes: N/A
Fixes #9859
Fixes #12140