extensions: add retry host predicate for attempted hosts #4452
Conversation
Adds a simple RetryHostPredicate that keeps track of hosts that have already been attempted, triggering a new host to be selected if an already attempted host is selected. Signed-off-by: Snow Pettersen <snowp@squareup.com>
| class OtherHostsRetryPredicateFactory : public Upstream::RetryHostPredicateFactory { | ||
| public: | ||
| void createHostPredicate(Upstream::RetryHostPredicateFactoryCallbacks& callbacks, | ||
| const ProtobufWkt::Struct&) override { |
There was a problem hiding this comment.
Let's continue this discussion in #4475. We do want to move in this direction, but we want to maintain a consistent and unsurprising API surface for extensions.
There was a problem hiding this comment.
I think it makes sense to do in a follow up, that way I can do both extensions at the same time without cluttering this PR
There was a problem hiding this comment.
Actually, considering #4385 has still not been reviewed (and this introduces the other Struct usage) I think I'll just do it in each PR separately
There was a problem hiding this comment.
Turns out it's more elaborate than I thought - I'll update this PR to use MessagePtr instead of Struct and then follow up with another PR setting up a base factory to convert that into a config proto. This PR isn't relying on parsing a config proto, so I think it can be merged without having all that set up
Sorry for being so indecisive!
Signed-off-by: Snow Pettersen <snowp@squareup.com>
Signed-off-by: Snow Pettersen <snowp@squareup.com>
| } | ||
|
|
||
| private: | ||
| std::unordered_set<std::string> attempted_hosts_; |
There was a problem hiding this comment.
@snowp I'm not sure of my comment came through here or not: I suspect that a vector or fixed size array initialized to the number of max retries would end up being faster here and use less memory given the generally tiny number of allowed retries. Something to think about / TODO in the future.
There was a problem hiding this comment.
That sounds like a good idea. I think the only complication is having to plumb through the max retries count, but since other implementations might also find it useful for similar use cases it should probably be part of the factory interface.
Adds a simple RetryHostPredicate that keeps track of hosts that have
already been attempted, triggering a new host to be selected if an
already attempted host is selected.
Signed-off-by: Snow Pettersen snowp@squareup.com
Risk Level: Low
Testing: Unit test
Docs Changes: n/a
Release Notes: n/a
Part of #3958