-
Notifications
You must be signed in to change notification settings - Fork 947
feat(retry): add configurable link recovery delay #25630
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a configurable link recovery delay feature to the Azure Event Hubs SDK, addressing issue #25356. The feature allows users to specify a custom delay for retries after AMQP link recovery failures, instead of using the default exponential backoff.
Key changes:
- Added
LinkRecoveryDelayfield toRetryOptionsto configure fixed delays for link recovery retries - Enhanced
RetryFnArgswithUseLinkRecoveryDelay()method to signal when link recovery delay should be used - Updated retry logic to apply the link recovery delay when appropriate, with support for zero (use exponential backoff), positive (fixed delay), and negative (no delay) values
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/messaging/azeventhubs/internal/exported/retry_options.go | Added LinkRecoveryDelay field to RetryOptions with comprehensive documentation |
| sdk/messaging/azeventhubs/internal/utils/retrier.go | Implemented logic to use link recovery delay when signaled, added UseLinkRecoveryDelay() method |
| sdk/messaging/azeventhubs/internal/utils/retrier_test.go | Added comprehensive test coverage for link recovery delay scenarios |
| sdk/messaging/azeventhubs/internal/links_recover.go | Updated RecoverIfNeeded() to accept RetryFnArgs and call UseLinkRecoveryDelay() for link recovery |
| sdk/messaging/azeventhubs/internal/links_unit_test.go | Updated all RecoverIfNeeded() calls to pass RetryFnArgs parameter |
| sdk/messaging/azeventhubs/internal/links_test.go | Updated all RecoverIfNeeded() calls to pass RetryFnArgs parameter |
| sdk/messaging/azeventhubs/CHANGELOG.md | Added entry documenting the new configurable link recovery delay feature |
Comments suppressed due to low confidence (1)
sdk/messaging/azeventhubs/internal/utils/retrier.go:100
- When
ResetAttempts()is called, theuseLinkRecoveryDelayflag is not being reset. This means if a link recovery delay was set before callingResetAttempts(), it will continue to be applied in the subsequent retry attempts after the reset, which may not be the intended behavior. Consider resettinguseLinkRecoveryDelayto false whenargs.resetAttemptsis true, similar to how the iteration counter is reset.
// Capture the flag for the next iteration
useLinkRecoveryDelay = args.useLinkRecoveryDelay
if args.resetAttempts {
log.Writef(eventName, "(%s) Resetting retry attempts", prefix())
// it looks weird, but we're doing -1 here because the post-increment
// will set it back to 0, which is what we want - go back to the 0th
// iteration so we don't sleep before the attempt.
//
// You'll use this when you want to get another "fast" retry attempt.
i = int32(-1)
}
7f3e119 to
f908602
Compare
f908602 to
6414ec8
Compare
Allows for configuration which fixes the issue: #25356