router: implement RetryHostPredicate #4385
Conversation
Wires up route configuration to allow specifying what hosts should be reattempted during retry host selection. Signed-off-by: Snow Pettersen <snowp@squareup.com>
Signed-off-by: Snow Pettersen <snowp@squareup.com>
|
@zuercher If you have time I'd love some feedback on this one. Part of the implementation of the previous retry PR you looked over. |
| #include "envoy/registry/registry.h" | ||
|
|
||
| namespace Envoy { | ||
| static Registry::RegisterFactory<TestHostPredicateFactory, Upstream::RetryHostPredicateFactory> |
There was a problem hiding this comment.
You can use Registery::InjectFactory to avoid having to insure this factory is linked into your tests and also control exactly when the predicate is available.
Signed-off-by: Snow Pettersen <snowp@squareup.com>
|
Build failure seems to be due to CircleCi degraded service, I'll re-run CI in a bit once they resolve their issues |
source/common/router/config_impl.cc
Outdated
| auto factory = Registry::FactoryRegistry<Upstream::RetryHostPredicateFactory>::getFactory( | ||
| host_predicate.name()); | ||
|
|
||
| ASSERT(factory); |
There was a problem hiding this comment.
Nit: no need to ASSERT if doing an immediate dereference next line.
source/common/router/config_impl.cc
Outdated
| @@ -48,6 +48,15 @@ RetryPolicyImpl::RetryPolicyImpl(const envoy::api::v2::route::RouteAction& confi | |||
| num_retries_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT(config.retry_policy(), num_retries, 1); | |||
| retry_on_ = RetryStateImpl::parseRetryOn(config.retry_policy().retry_on()); | |||
| retry_on_ |= RetryStateImpl::parseRetryGrpcOn(config.retry_policy().retry_on()); | |||
|
|
|||
| for (const auto& host_predicate : config.retry_policy().retry_host_predicate()) { | |||
| // TODO(snowp): support passing the config Struct during initialization. | |||
There was a problem hiding this comment.
Seems reasonable to do in this PR, should only be a few lines?
test/integration/http_integration.h
Outdated
| void waitForNextUpstreamRequest(uint64_t upstream_index = 0); | ||
| // In cases where the upstream that will receive the request is not deterministic, a second | ||
| // upstream index may be provided, in which case both upstreams will be checked for requests. | ||
| uint64_t waitForNextUpstreamRequest(uint64_t upstream_index = 0, |
There was a problem hiding this comment.
If feels a bit clunky making this either 1 or 2; why not just provide a set of hosts?
| @@ -308,5 +308,11 @@ class MockClusterInfoFactory : public ClusterInfoFactory, Logger::Loggable<Logge | |||
| Event::Dispatcher& dispatcher, Runtime::RandomGenerator& random)); | |||
| }; | |||
|
|
|||
| class MockRetryHostPredicate : public RetryHostPredicate { | |||
| public: | |||
There was a problem hiding this comment.
Please add explicit constructor/destructor and put definition in .cc for mock build performance reasons.
There was a problem hiding this comment.
Surprised this makes a difference. Does the implicit ctor/dtor definition in header add significant overhead?
There was a problem hiding this comment.
Surprising to me too, but see https://github.com/google/googletest/blob/master/googlemock/docs/CookBook.md#making-the-compilation-faster.
There was a problem hiding this comment.
Interesting, thanks for the link. Makes me think we should go over the existing mocks and move the ctors/dtors into the .cc file, I see it's missing from some of them
There was a problem hiding this comment.
Sure, any cleanups PRs most welcome.
Signed-off-by: Snow Pettersen <snowp@squareup.com>
test/integration/http_integration.cc
Outdated
| absl::optional<uint64_t> second_upstream_index) { | ||
| uint64_t upstream_with_request = upstream_index; | ||
| uint64_t HttpIntegrationTest::waitForNextUpstreamRequest( | ||
| const std::unordered_set<uint64_t>& upstream_indices) { |
There was a problem hiding this comment.
I think you want this to be a vector or you're going to have a source of non-determinism leading to test flakes.
There was a problem hiding this comment.
Ah that's a good point, I'll change
Signed-off-by: Snow Pettersen <snowp@squareup.com>
Description: Wires up route configuration to allow specifying what hosts should be
reattempted during retry host selection.
Risk Level: Medium, some changes made to the router. Otherwise new optional feature
Testing: unit and integration test
Docs Changes: n/a
Release Notes: n/a
Part of #3958