filter: Add HttpCache interface and helpers#9878
filter: Add HttpCache interface and helpers#9878mattklein123 merged 4 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Todd Greer <tgreer@google.com>
Signed-off-by: Todd Greer <tgreer@google.com>
|
I messed up some of the protoc magic. I'll take another stab at it as soon as I'm back in the office or otherwise find time--sorry for the premature review request. |
jmarantz
left a comment
jmarantz
left a comment
There was a problem hiding this comment.
looks good modulo 1 nit; will need API approval though.
I have a question about that -- really for @htuch or other @envoyproxy/senior-maintainers -- this is an extension but there is a modification here to what appears to be the core Envoy API. Should we have a different directory for the API provided by an extension, that doesn't require API maintainer review?
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Quick high level question, thank you.
/wait
Signed-off-by: Todd Greer <tgreer@google.com>
toddmgreer
changed the title
|
PTAL. |
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
I need to run but flushing out one comment now and will look more later. Very cool stuff!
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Thanks LGTM from a high level skim. Just some small random comments. Thank you!
/wait
| // Headers of the cached response. | ||
| Http::HeaderMapPtr headers_; |
There was a problem hiding this comment.
This will require copying the headers for every cached response, which seems not-optimal. Since this is in the encoding path and I think we encode with a const ref do we need to copy here?
There was a problem hiding this comment.
StreamDecoderFilterCallbacks::encodeHeaders takes a HeaderMapPtr&&, so I don't think I have any alternatives. It seems fine though: we're going to have to create a HeaderMap for the cache entry; handing its ownership to the callbacks makes sense.
There was a problem hiding this comment.
OK but then we have to needlessly copy on every cached hit response? In any case, we can deal with this later, but maybe put in a perf TODO somewhere relevant?
There was a problem hiding this comment.
I'm not seeing where the needless copy is. Cache storage implementations will generally store a serialized form of the response headers. When ready to serve an entry, they'll create a HeaderMap from it, owned by a HeaderMapPtr, which they'll give to encodeHeaders. Implementations might copy the headers into the HeaderMap, or they might create a new subclass that references the serialized form. If an implementation chooses zero-copy, these interfaces won't get in the way.
There was a problem hiding this comment.
It seems like an in-memory implementation could store the actual HeaderMap object. In any case, we can deal with this later.
Signed-off-by: Todd Greer <tgreer@google.com>
|
Thank you for the rapid reviews! |
3 participants
Description: Adds HttpCache, the pluggable interface for CacheFilter (#868, #7198)
Risk Level: Low. Nothing calls or any of the new code.
Testing: Unit tests.
Docs Changes: none.
Release Notes: N/A
#868