extenstions: retry predicate - omit_canary_hosts#7230
extenstions: retry predicate - omit_canary_hosts#7230snowp merged 7 commits intoenvoyproxy:masterfrom
Conversation
snowp
left a comment
There was a problem hiding this comment.
Thanks for working on this! This seems like exactly what I was thinking, just have some comments about style/conventions.
You'll also want to fix DCO (https://github.com/envoyproxy/envoy/blob/master/CONTRIBUTING.md#fixing-dco) and format the files using tools/check_format.py fix.
There was a problem hiding this comment.
Since this extension lives in the envoy repo we'll want to prefix the name with envoy like all the other extensions
There was a problem hiding this comment.
Use std::make_unique instead of explicit new
There was a problem hiding this comment.
Sure! I got started on this PR by copying the sources of the previous_hosts extensions, can I make this change there also?
There was a problem hiding this comment.
Sure, I wouldn't mind the cleanup
There was a problem hiding this comment.
this does nothing, you can just remove it
…nvoyproxy#7203) In case a user wants to deploy a new change, this can help in cases where any failure in the canary downstream is retried in a stable downstream. Signed-off-by: Sriduth Jayhari <sriduth.jayhari@juspay.in>
…anary_hosts` retry predicate in docs/root/intro/arch_overview/http_connection_management.rst Signed-off-by: Sriduth Jayhari <sriduth.jayhari@juspay.in>
e6e7f4e to
06e27c4
Compare
06e27c4 to
266f00f
Compare
snowp
left a comment
There was a problem hiding this comment.
Nice this looks pretty good. Just a few minor comments. Could you add a release note for this as well?
* Added release note * Improve documentation * return unique_ptr of Envoy::ProtobufWkt::Empty instead of wrapping with a message ptr in createEmptyProto (also added for previous hosts predicate) Signed-off-by: Sriduth Jayhari <sriduth.jayhari@juspay.in>
|
This looks good to me. Could you merge master and see if CI is okay? You can also move this out of Draft mode at this point. |
|
Seems like there's a CI failure /wait |
2ba14cd to
3d1bc6b
Compare
|
@snowp , running Should I revert the last 2 commits (format fix and master merge)? |
|
Yeah please avoid changing files not related to this change. I tend to run the format script just on the files I've changed in a PR instead of on the whole repo to avoid this kinda stuff from happening. |
Signed-off-by: Sriduth Jayhari <sriduth.jayhari@juspay.in>
3d1bc6b to
93e440e
Compare
|
I should have checked the help section of the check_format.py script - my bad! I've run the following and no diff is produced:
|
|
Should I add this to the |
|
Yeah, please add yourself and me as the code owners. |
Signed-off-by: Sriduth Jayhari <sriduth.jayhari@juspay.in>
snowp
left a comment
There was a problem hiding this comment.
Thanks this is looking really good, just a couple nits.
* Remove unrequired empty line in documentation for the predicate in http_connection_management.rst * Improve wording and formatting that describes change in version_history.rst Signed-off-by: Sriduth Jayhari <sriduth.jayhari@juspay.in>
1b31992 to
21d3b9b
Compare
…oyproxy#7230 Signed-off-by: Sriduth Jayhari <sriduth.jayhari@juspay.in>
… (#7394) Signed-off-by: Sriduth Jayhari <sriduth.jayhari@juspay.in>
|
It looks like this feature was added to the version |
|
@mcasper you are right! i created the feature branch off the v.1.10.0 commit. This is bad - I'll open a PR to fix this right away. |
…r v1.11 version
history.
Signed-off-by: Sriduth Jayhari <sriduth.jayhari@juspay.in>
…rsion (#7444) history. Signed-off-by: Sriduth Jayhari <sriduth.jayhari@juspay.in>
Description: Created a new retry predicate that will reject canary hosts when selecting a host to retry a failure.
Risk Level: Low
Testing: Unit Tests added, manual testing done (1 canary host, 1 non canary host)
Docs Changes: Added brief description in
docs/root/intro/arch_overview/http_connection_management.rstRelease Notes: N/A
[Optional Fixes #Issue] Fixes #7203
[Optional Deprecated:]