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

Flip grpc client cache flag #19253

Merged
merged 7 commits into from
Jan 18, 2022

Conversation

chaoqin-li1123
Copy link
Member

Commit Message: Make grpc client cache default enable.
Additional Description:
Risk Level: low
Testing: change testing to verify new behavior
Docs Changes: document the behavior change in current.rst
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

@phlax
Copy link
Member

phlax commented Dec 13, 2021

/retest

@repokitteh-read-only
Copy link

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

🐱

Caused by: a #19253 (comment) was created by @phlax.

see: more, trace.

Copy link
Contributor

@jmarantz jmarantz left a comment

Choose a reason for hiding this comment

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

Code looks fine; I thought we were going to enable this at one call-site per PR though.

Or is there only one remaining call-site at this point?

/wait

@chaoqin-li1123
Copy link
Member Author

Per our discussion, it is ok to flip a runtime even when involves many components and filters.

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had activity in the last 30 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!

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Jan 13, 2022
@alyssawilk alyssawilk removed stale stalebot believes this issue/PR has not been touched recently waiting labels Jan 13, 2022
@@ -14,6 +14,7 @@ Minor Behavior Changes
* bandwidth_limit: added :ref:`response trailers <envoy_v3_api_field_extensions.filters.http.bandwidth_limit.v3.BandwidthLimit.enable_response_trailers>` when request or response delay are enforced.
* bandwidth_limit: added :ref:`bandwidth limit stats <config_http_filters_bandwidth_limit>` *request_enforced* and *response_enforced*.
* dns: now respecting the returned DNS TTL for resolved hosts, rather than always relying on the hard-coded :ref:`dns_refresh_rate. <envoy_v3_api_field_config.cluster.v3.Cluster.dns_refresh_rate>` This behavior can be temporarily reverted by setting the runtime guard ``envoy.reloadable_features.use_dns_ttl`` to false.
* grpc: flip runtime guard ``envoy.reloadable_features.enable_grpc_async_client_cache`` to be default enabled.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make sure comment what the functional change is?

Also this got lost as well due to the waiting tag, sorry. I've removed it so hopefully it will make progress again, but you'll want to main merge for current.rst

@jmarantz
Copy link
Contributor

also needs main merge

chaoqin-li1123 added 3 commits January 15, 2022 03:24
Signed-off-by: chaoqin-li1123 <[email protected]>
Signed-off-by: chaoqin-li1123 <[email protected]>
@chaoqin-li1123
Copy link
Member Author

./retest

@chaoqin-li1123
Copy link
Member Author

/retest

@repokitteh-read-only
Copy link

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

🐱

Caused by: a #19253 (comment) was created by @chaoqin-li1123.

see: more, trace.

Copy link
Contributor

@jmarantz jmarantz left a comment

Choose a reason for hiding this comment

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

Needs a non-Google senior maintainer approval:

/assign-from @envoyproxy/senior-maintainers

@repokitteh-read-only
Copy link

@envoyproxy/senior-maintainers assignee is @zuercher

🐱

Caused by: a #19253 (review) was submitted by @jmarantz.

see: more, trace.

@zuercher zuercher merged commit 2d616e9 into envoyproxy:main Jan 18, 2022
joshperry pushed a commit to joshperry/envoy that referenced this pull request Feb 13, 2022
Risk Level: low
Testing: change testing to verify new behavior
Docs Changes: n/a
Release Notes: added
Platform Specific Features: n/a

Signed-off-by: chaoqin-li1123 <[email protected]>
Signed-off-by: Josh Perry <[email protected]>
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.

5 participants