Skip to content

Conversation

@vroldanbet
Copy link
Contributor

@vroldanbet vroldanbet commented Apr 24, 2025

Introduces gRPC middleware to do client-side retries by default. The only flag introduced to customize this behaviour is the maximum number of attempts, which defaults to 20.

Retries are enabled for both unary and streaming bot APIs.

retries_and_resume_demo.mp4

Copy link
Contributor

@tstirrat15 tstirrat15 left a comment

Choose a reason for hiding this comment

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

See comments

Comment on lines +40 to +43
defaultRetryExponentialBackoff = 100 * time.Millisecond
defaultMaxRetryAttemptDuration = 2 * time.Second
defaultRetryJitterFraction = 0.5
)
Copy link
Contributor

Choose a reason for hiding this comment

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

If the upstream is just straight-up unavailable (say you fatfingered your port), will 20 retries + this configuration mean that zed appears to hang for a long time? Should we have a smaller number of retries by default?

Copy link
Contributor Author

@vroldanbet vroldanbet Apr 24, 2025

Choose a reason for hiding this comment

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

You can terminate the command manually in that case, but I'm ok changing the default number of retries. What do you think would be a sensible value? I've dialed it down to 10

Copy link
Contributor

Choose a reason for hiding this comment

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

We chatted on this and 10 retries seems like a sane number.

@vroldanbet vroldanbet changed the base branch from improve-validation-errors to main April 24, 2025 16:41
Introduces gRPC middleware to do client-side retries by default.
The only flag introduced to customize this behaviour is the maximum
number of attempts, which defaults to 20.

The retries are enabled for bot unary and streaming APIs.
@vroldanbet vroldanbet enabled auto-merge April 24, 2025 16:48
@vroldanbet vroldanbet requested a review from tstirrat15 April 24, 2025 16:50
Copy link
Contributor

@tstirrat15 tstirrat15 left a comment

Choose a reason for hiding this comment

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

LGTM!

@vroldanbet vroldanbet merged commit 1b2c8eb into main Apr 24, 2025
11 checks passed
@vroldanbet vroldanbet deleted the add-retries branch April 24, 2025 17:03
@github-actions github-actions bot locked and limited conversation to collaborators Apr 24, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants