grpc: standardize the cache interface of grpc async client.#15745
grpc: standardize the cache interface of grpc async client.#15745jmarantz merged 62 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>
…act_async_client_cache
…act_async_client_cache
Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>
Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>
Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>
Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>
rojkov
left a comment
There was a problem hiding this comment.
Thank you! I'm not very familiar with the code, but left some driveby comments.
test/extensions/filters/http/ratelimit/ratelimit_integration_test.cc
Outdated
Show resolved
Hide resolved
Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>
|
@adisuissa @dmitri-d I think you folks might be interested in taking a pass on this from a control plane perspective. |
|
This needs a non-google review I think. @lizan can you take a look? |
|
Thanks @lizan . @chaoqin-li1123 can you merge main? |
|
/wait |
…act_async_client_cache
…act_async_client_cache
Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>
Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>
Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>
|
@jmarantz @htuch @chaoqin-li1123 sorry late to this PR, but I see a lot of comments about cached vs. uncached. From a skim of the PR I'm not quite sure why some users were converted to cached and some were not. Can we at minimum backfill TODO comments or other comments on every callsite using uncached to explain why that is happening? From reading it quickly it's confusing. Thank you. |
I think ultimately we want to use the cached version everywhere. But for now only the grpc client for ex_authz is default enabled, which is equivalent to the old behavior. We can open an issue as followup and clean this up in the following pull request. |
Please go through and add comments/TODOs on each of the usages when you open the issue. Thank you! |
|
Yes, this was also my question but I think the position of wanting to derisk this by starting with |
- no changes in `.bazelrc`, `.bazelversion` or `ci/run_envoy_docker.sh`. - reverting the `addIdleCallback` connection pool method back to `addDrainedCallback` as per envoyproxy/envoy#17319. - changing method call `AsyncClientFactory::create()` to `AsyncClientFactory::createUncachedRawAsyncClient()` as per envoyproxy/envoy#15745. Signed-off-by: Jakub Sobon <mumak@google.com>
- no changes in `.bazelrc`, `.bazelversion` or `ci/run_envoy_docker.sh`. - reverting the `addIdleCallback` connection pool method back to `addDrainedCallback` as per envoyproxy/envoy#17319. - changing method call `AsyncClientFactory::create()` to `AsyncClientFactory::createUncachedRawAsyncClient()` as per envoyproxy/envoy#15745. Signed-off-by: Jakub Sobon <mumak@google.com>
…xy#15745) Commit Message: Creating a grpc async client can be expensive. Caching and reusing grpc async client can save resources and improve stats consistency. This pr add a thread local cache of grpc async client as a member of grpc async client manager. Additional Description:N/A Risk Level:medium Testing:add unit and integration tests Docs Changes:N/A Release Notes: Platform Specific Features: Fixes: envoyproxy#12598 Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>
|
Checking back in as we cut the release, what's the plan for flipping this true? |
|
We finish the migration for ext_authz and ext_proc. |
Commit Message: Creating a grpc async client can be expensive. Caching and reusing grpc async client can save resources and improve stats consistency. This pr add a thread local cache of grpc async client as a member of grpc async client manager.
Additional Description:N/A
Risk Level:medium
Testing:add unit and integration tests
Docs Changes:N/A
Release Notes:
Platform Specific Features:
Fixes: #12598