-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
filter: Add an incomplete pluggable HTTP caching filter (CacheFilter). #7198
Conversation
…proxy#868). Signed-off-by: Todd Greer <tgreer@google.com>
…proxy#868) Signed-off-by: Todd Greer <tgreer@google.com>
Signed-off-by: Todd Greer <tgreer@google.com>
Signed-off-by: Todd Greer <tgreer@google.com>
Signed-off-by: Todd Greer <tgreer@google.com>
Signed-off-by: Todd Greer <tgreer@google.com>
Signed-off-by: Todd Greer <tgreer@google.com>
@envoyproxy/maintainers any volunteers to shepherd review on this? I don't have the bandwidth to take this on. |
Signed-off-by: Todd Greer <tgreer@google.com>
Signed-off-by: Todd Greer <tgreer@google.com>
/retest |
🐴 hold your horses - no failures detected, yet. |
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
/wait |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flushing comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I think I'm done with this pass; can you address these and I'll dig deeper?
* source/common/http/headers.h: Signed-off-by: Todd Greer <tgreer@google.com>
Signed-off-by: Todd Greer <tgreer@google.com>
Signed-off-by: Todd Greer <tgreer@google.com>
Signed-off-by: Todd Greer <tgreer@google.com>
Signed-off-by: Todd Greer <tgreer@google.com>
Signed-off-by: Todd Greer <tgreer@google.com>
Signed-off-by: Todd Greer <tgreer@google.com>
Signed-off-by: Todd Greer <tgreer@google.com>
Signed-off-by: Todd Greer <tgreer@google.com>
… ref. Signed-off-by: Todd Greer <tgreer@google.com>
Hi @toddmgreer, I have implemented http cache backed by Hazelcast IMDG with the completed features on the filter side (repository). I’m planning to make some changes (i.e. formatting the code style to make it consistent with Envoy cpp style, etc. ) but the caching logic will be more or less the same. What about using it as a reference implementation or the default provider? I believe implementing the filter hand in hand with a provider would make sense and once the filter is ready for production, a plugin can be provided as well. |
/azp run envoy-linux |
Azure Pipelines successfully started running 1 pipeline(s). |
Hi @enozcan, that's fantastic--I'm excited to see new work building on CacheFilter! HazelcastHttpCache seems like a perfect candidate to be included with Envoy (though that will ultimately be up to Envoy's senior maintainers). I don't intend for there to be a default provider / HttpCache implementation--the configuration should specify something, but HazelcastHttpCache makes perfect sense as one of the built-in options. Any complaints about the interfaces? Were they clear enough? Thank you for extending Envoy/CacheFilter! |
/retest |
🐴 hold your horses - no failures detected, yet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, left some more comments. At a high level this LGTM. TBH my advice for getting this merged at this point is to start splitting this into small PRs that we can re-review without the massive size of this PR. You could easily start with a PR for the utilities, then the cache itself, then the filter, then the simple cache implementation, etc. I think if we do that @jmarantz and I can commit to efficiently re-reviewing and we can land this? WDYT? Thank you for all your work here.
/wait
|
||
CacheFilterSharedPtr self = shared_from_this(); | ||
ENVOY_STREAM_LOG(debug, "CacheFilter::decodeHeaders starting lookup", *decoder_callbacks_); | ||
lookup_->getHeaders([self](LookupResult&& result) { onHeadersAsync(self, std::move(result)); }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this async lambda ever fire inline? I could imagine it can and we need to deal with this case?
Http::FilterDataStatus CacheFilter::encodeData(Buffer::Instance& data, bool end_stream) { | ||
if (insert_) { | ||
ENVOY_STREAM_LOG(debug, "CacheFilter::encodeHeaders inserting body", *encoder_callbacks_); | ||
// TODO(toddmgreer): Wait for the cache if necessary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to waiting for the cache, you will also have to apply back pressure potentially. Can you make sure this is covered by an open issue?
case CacheEntryStatus::RequiresValidation: | ||
case CacheEntryStatus::FoundNotModified: | ||
case CacheEntryStatus::UnsatisfiableRange: | ||
ASSERT(false); // We don't yet return or support these codes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switch to NOT_IMPLEMENTED macro
ASSERT(false, "CacheFilter doesn't call getBody unless there's more body to get, so this is a " | ||
"bogus callback."); | ||
decoder_callbacks_->resetStream(); | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't add error handling for cases like this. A plain ASSERT is fine before proceeding. If something can't happen I would just assert it and move on. Same below. If we want to actually defend against bad cache implementations then I would remove the ASSERT(falses) statements throughout and actually handle things, and probably have stats eventually (so add a TODO for that).
void CacheFilter::onDestroy() { | ||
lookup_ = nullptr; | ||
insert_ = nullptr; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a small comment here that we assume that destroying the handles cancels all async callbacks, etc.?
namespace Cache { | ||
|
||
class CacheFilterFactory | ||
: public Common::FactoryBase<envoy::config::filter::http::cache::v2::CacheConfig> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK not sure what is going on here. I think @htuch is out this week so if this isn't burning urgent we should ask him next week before merging.
asctime | ||
dechunk | ||
dechunked | ||
qdtext |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. There is already an issue tracking this which we should just implement in the checker.
HI @toddmgreer,
Overall everything was fine but I wonder a few things. I followed the design doc during implementation but it seems like out of date. Some updates are not reflected on the doc. If it is planned to be maintained, I can help to make it up-to-date by suggesting editions on the doc page. Also, as stated in the previous comments, vary headers issue is a little bit complicated. A sequence diagram for it just like the existing one in the doc might be extremely helpful for providers. Also, in
Which directory should I address when including a new plugin in Envoy repository? May be a subdirectory like |
Hi Enes,
Thank you! I'm glad to hear the interfaces make sense. I hope the stale
design doc didn't cause you much inconvenience--sorry about
that. @mattklein123 indicated that it's preferred to include docs in the
repo instead of linking to external docs, so updating should happen there.
As the functionality gaps get plugged, the docs will need to be updated--I
agree that diagrams from the design doc will probably need to be copied
over.
As you say, having a subdirectory per plugin
(e.g. filters/http/cache/hazelcast_http_cache), seems like a good approach.
I'm going to move SimpleHttpCache to its own dir shortly. You should be
able to add HazelcastHttpCache as soon as PR #9878 lands. As you find
things that need improvement, please continue to let me know, file bugs,
and/or submit PRs.
…On Fri, Feb 7, 2020 at 6:07 AM Enes Ozcan ***@***.***> wrote:
HI @toddmgreer <https://github.com/toddmgreer>,
Any complaints about the interfaces? Were they clear enough?
Overall everything was fine but I wonder a few things. I followed the design
doc
<https://docs.google.com/document/d/1WPuim_GzhfdsnIj_tf-fIeutK0jO4aVQfVrLJFoLN3g/view>
during implementation but it seems like out of date. Some updates are not
reflected on the doc. If it is planned to be maintained, I can help to make
it up-to-date by suggesting editions on the doc page. Also, as stated in
the previous comments, vary headers issue is a little bit complicated. A
sequence diagram for it just like the existing one in the doc might be
extremely helpful for providers.
Also, in source/docs/cache_filter_plugins.md it's stated that:
If you write a new cache storage implementation, please add it to the Envoy
repository if possible.
Which directory should I address when including a new plugin in Envoy
repository? May be a subdirectory like
filters/http/cache/hazelcast_http_cache? Also the documentation for the
plugin, is source/docs/ the right place?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7198?email_source=notifications&email_token=AFRAWPO4QWHGBAWAA4RO2PLRBVTKBA5CNFSM4HVNTKI2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELDANUQ#issuecomment-583403218>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFRAWPPPZP4IIQ3LKGQR6SLRBVTKBANCNFSM4HVNTKIQ>
.
|
All of the changes in this PR have now been merged through other PRs, with the exception of support for multithreaded cache storage implementations (removed per reviewer request in #10019). |
…nvoyproxy#7198 Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
Description: Adds CacheFilter, an HTTP filter that caches response. To support a variety of needs, storage is delegated to a plugin cache implementation, but CacheFilter handles the detailed logic of determining what can be cached, whether to serve a particular cache entry for a particular request, and validation.
Design: eCache: a multi-backend HTTP cache for Envoy
This initial submission establishes the framework for cache implementations (including a simple example plugin), and handles basic integration with Envoy as an HTTP filter. It doesn't yet handle enough of the RFCs to be used, and the tests only validate basic functionality. While there is substantial work remaining, merging this now will make it easier for development of cache plugins to happen in wile the filter side is finished.
Risk Level: Low. Since there are no (non-test) uses of this new filter, it would be difficult for it to break anything. The only changes to actually executed code are two new HeaderMap inline headers and the new static factory registration.
Testing: All components have basic unit tests. Integration tests and more unit tests will be needed before CacheFilter is ready for production use.
Docs Changes: The new config proto (envoy.config.filter.http.cache.v2alpha.Cache) has reasonable documentation, and has been added to docs/build.sh.
Release Notes: N/A
#868