Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add retry backoff interval #3461

Merged
merged 2 commits into from
Mar 20, 2025

Conversation

ramaraochavali
Copy link
Contributor

Configures retry backoff interval. Needed for gateway api also

Signed-off-by: Rama Chavali <[email protected]>
@ramaraochavali ramaraochavali requested a review from a team as a code owner March 8, 2025 11:16
@ramaraochavali ramaraochavali added the release-notes-none Indicates a PR that does not require release notes. label Mar 8, 2025
@istio-testing istio-testing added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 8, 2025
@@ -1317,6 +1317,9 @@ message HTTPRetry {
// Flag to specify whether the retries should ignore previously tried hosts during retry.
// Defaults to true.
google.protobuf.BoolValue retry_ignore_previous_hosts = 5;

// Specifies the minimum duration between retry attempts.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we indicate something along the lines of "if unset, retries will be instant"? And maybe clarify the interaction with a timeout (e.g. what if my backoff=2s, timeout=3s, retries=3 - what happens? I assume we can retry at most 1x?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Updated. PTAL

Signed-off-by: Rama Chavali <[email protected]>
@ramaraochavali
Copy link
Contributor Author

@howardjohn can PTAL when you get chance?

Copy link
Member

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM aside from default comment

@@ -1317,6 +1317,13 @@ message HTTPRetry {
// Flag to specify whether the retries should ignore previously tried hosts during retry.
// Defaults to true.
google.protobuf.BoolValue retry_ignore_previous_hosts = 5;

// Specifies the minimum duration between retry attempts.
// If unset, default minimum duration of 25ms is used as base interval for exponetial backoff.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the default no backoff?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh. TIL. I always thought it was instant

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it valid to set 0s? I think our standard valdiation will reject it unless we add protoc-gen-crd:duration-validation:none.

Note the GW API says "if unset its implementation specific" so we can keep that at 25ms, but they do allow 0s i think

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be greater than 0s in Envoy. If gateway allows that, we will have to tweak that in Envoy.

@istio-testing istio-testing merged commit 81bc190 into istio:master Mar 20, 2025
5 checks passed
@ramaraochavali ramaraochavali deleted the fix/retry_backoff branch March 20, 2025 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes-none Indicates a PR that does not require release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants