Skip to content
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

alternate_protocols_cache: Impose a max size limit on the alternate protocols cache #18258

Merged
merged 8 commits into from
Sep 28, 2021

Conversation

RyanTheOptimist
Copy link
Contributor

alternate_protocols_cache: Impose a max size limit on the alternate protocols cache

Implement a very basic MRU map in AlternateProtocolsCacheImpl.

Risk Level: Low
Testing: Unit tests
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

…rotocols cache

Implement a very basic MRU map in AlternateProtocolsCacheImpl.

Signed-off-by: Ryan Hamilton <rch@google.com>
@RyanTheOptimist
Copy link
Contributor Author

/assign @RenjieTang

Copy link
Contributor

@RenjieTang RenjieTang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good!
Looks like some tests need fixing?

source/common/http/alternate_protocols_cache_impl.h Outdated Show resolved Hide resolved
Copy link
Contributor Author

@RyanTheOptimist RyanTheOptimist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also fixed the failing test (by initializing max_entries_ to 1024, as specified in the proto, when the argument is 0.

source/common/http/alternate_protocols_cache_impl.h Outdated Show resolved Hide resolved
Signed-off-by: Ryan Hamilton <rch@google.com>

Signed-off-by: Ryan Hamilton <rch@google.com>
Signed-off-by: Ryan Hamilton <rch@google.com>
Signed-off-by: Ryan Hamilton <rch@google.com>
RenjieTang
RenjieTang previously approved these changes Sep 27, 2021
@RyanTheOptimist
Copy link
Contributor Author

/assign @alyssawilk

Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for being up for tackling this TODO!

// List of origin, alternate protocol pairs, in insertion order.
std::list<OriginProtocols> protocols_list_;
// Map from hostname to iterator into protocols_list_.
std::map<Origin, std::list<OriginProtocols>::iterator> protocols_map_;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we already depend on quiche for the spdy wire parsing, what do you think of also pulling in quiche's linked hash map?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, good call! I didn't even consider that. It's a much simpler change. Thanks! Done.

Signed-off-by: Ryan Hamilton <rch@google.com>
Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo the one remaining question :-P

struct OriginHash {
size_t operator()(const Origin& origin) const {
size_t hash = std::hash<std::string>()(origin.scheme_) +
37 * (std::hash<std::string>()(origin.hostname_) +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

37?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I stole this from source/common/upstream/load_balancer_impl.h which seems to be based on a pattern that's referenced in Effective Java (a great book). Good enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also added a comment.

Signed-off-by: Ryan Hamilton <rch@google.com>
Signed-off-by: Ryan Hamilton <rch@google.com>
Signed-off-by: Ryan Hamilton <rch@google.com>
@RyanTheOptimist
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18258 (comment) was created by @RyanTheOptimist.

see: more, trace.

@alyssawilk alyssawilk merged commit 37329e3 into envoyproxy:main Sep 28, 2021
mpuncel added a commit to mpuncel/envoy that referenced this pull request Oct 29, 2021
* main: (114 commits)
  kafka: add header support to mesh-filter (envoyproxy#18248)
  rbac: add support for upstream ip policy. (envoyproxy#17645)
  SIPProxy BUGFIX UT failure for fastbuild/debug (envoyproxy#18303)
  quic: updating goaway code (envoyproxy#18291)
  various tiny fixes (envoyproxy#18287)
  dns cache: remove assert at this layer (envoyproxy#18301)
  [ext_authz]: ext_authz filter unit test that use real threading (envoyproxy#17742)
  signal action: fully disable sigaltstack on Apple (envoyproxy#18299)
  Add missing dependencies (envoyproxy#18297)
  ext_proc: Pass stream_info to gRPC streams (envoyproxy#18190)
  use clang 12 (envoyproxy#18220)
  Update PR template to include the "Fixes commit" message when reverting or fixing bad commits (envoyproxy#18298)
  [test] Fixing integration test to cleanup cleanly (envoyproxy#18293)
  test: moving grpc bridge tests out of core directory (envoyproxy#18227)
  runtime: disable deprecated extensions names by default (envoyproxy#18239)
  quiche: updating deps (envoyproxy#18272)
  sip_proxy: SIP protocol support in envoy (envoyproxy#18039)
  http: add core retry policy to route retry policy conversion utility (envoyproxy#17803)
  build: updating stale visibility (envoyproxy#18278)
  alternate_protocols_cache: Impose a max size limit on the alternate protocols cache (envoyproxy#18258)
  ...

Signed-off-by: Michael Puncel <mpuncel@squareup.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants