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 a SurfaceWorkErrors() opt to the retrier #51

Merged
merged 3 commits into from
Jul 19, 2024

Conversation

slizco
Copy link
Contributor

@slizco slizco commented May 7, 2024

If the retrier encounters a context-related error, such as context.DeadlineExceeded while attempting retries of work(), the context error is returned. This means the actual error from work() never bubbles up. Because the context is often used to enforce a timeout, many callers are likely more interested in the last non-nil error returned by the work() function.

We introduce a WithSurfaceWorkErrors() option to the retrier.Retrier that when used, means the last non-nil work error will be returned even if context-related errors are encountered.

Copy link
Owner

@eapache eapache left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! I’ve left a few nitpicks and testing suggestions, but I like the overall shape of this change quite a bit, and will be happy to merge it after a few tweaks.

retrier/retrier.go Outdated Show resolved Hide resolved
retrier/retrier.go Outdated Show resolved Hide resolved
retrier/retrier_test.go Outdated Show resolved Hide resolved
retrier/retrier_test.go Outdated Show resolved Hide resolved
@slizco
Copy link
Contributor Author

slizco commented Jul 18, 2024

@eapache Thank you for your review! I believe I addressed all your comments.

@eapache eapache merged commit 96d183b into eapache:main Jul 19, 2024
3 checks passed
@eapache
Copy link
Owner

eapache commented Jul 19, 2024

Thanks! Released in package version 1.7.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants