http: add core retry policy to route retry policy conversion utility#17803
http: add core retry policy to route retry policy conversion utility#17803jmarantz merged 16 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Shikugawa <rei@tetrate.io>
Shikugawa
changed the title
|
/assign-from @envoyproxy/maintainers for non-Tetrands review |
|
@envoyproxy/maintainers assignee is @jmarantz |
…tor-backoff-setup Signed-off-by: Shikugawa <rei@tetrate.io>
|
I assigned Ryan as a "first pass reviewer" to look at this PR first, then I can look at it next. This way we get more review mileage from prospective maintainers. |
RyanTheOptimist
left a comment
RyanTheOptimist
left a comment
There was a problem hiding this comment.
It looks like this code is moving from JwksFetcher to AsyncClient. However, AsyncClient lives in envoy/ and has very little logic. In general, envoy/ has basic structs and interfaces, but code lives elsewhere. The existing setRetryPolicy() method on AsynClient is a trivial setter which seems reasonable. This new method is quite non-trivial by comparison. I'm not sure this is a good fit for a place to live.
The bulk of the new method appears to be making a new route::RetryPolcy from core::RetryPolicy and configuring retries on it. Perhaps an alternative would be to write a method like:
route::RetryPolicy createRouteRetryPolicy(const core::retryPolicy& policy, bool retry_on);
This could be a static helper method somewhere. It would be good to write unit tests for this method too, I think. (The new method being added to AsyncClient has no tests, but the code in envoy/ typically doesn't because it's usually all trivial). What do you think?
The coverage appears to be failing. Do you know why?
Shikugawa
changed the title
RyanTheOptimist
left a comment
RyanTheOptimist
left a comment
There was a problem hiding this comment.
Two small comments, but looks good otherwise.
|
@lizan Could you take a look? |
|
@jmarantz mind doing a non-tetrands check? |
jmarantz
left a comment
jmarantz
left a comment
There was a problem hiding this comment.
lgtm modulo one nit/question.
jmarantz
left a comment
jmarantz
left a comment
There was a problem hiding this comment.
just a few nits, but I'll pass off to senior maintainers.
/assign-from @envoyproxy/senior-maintainers
|
@envoyproxy/senior-maintainers assignee is @lizan |
|
/retest |
|
Retrying Azure Pipelines: |
|
/retest |
|
Retrying Azure Pipelines: |
|
/retest |
|
Retrying Azure Pipelines: |
jmarantz
left a comment
jmarantz
left a comment
There was a problem hiding this comment.
Sorry this has bounced back & forth a few times :). I think we are all good on this PR if we can coax it through CI. It's failing on 2 tests in the context of coverage, which I've seen fail on other PRs also.
|
/retest |
|
Retrying Azure Pipelines: |
* 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>
7 participants
Signed-off-by: Shikugawa rei@tetrate.io
Commit Message: The conversion from
core::RetryPolicytoroute::RetryPolicyis essential for basic stream option setup. But in current impl, that is isolated on JWK fetcher and needed to use #17469. This refactor includes to the solution of them.Additional Description:
Risk Level: Low
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]