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 Backoff.ErrCause() #538

Merged
merged 3 commits into from
Jul 4, 2024
Merged

Conversation

pracucci
Copy link
Contributor

@pracucci pracucci commented Jul 3, 2024

What this PR does:

In this PR I propose a add Backoff.ErrCause() which is like Backoff.Err() but returns the context cause if backoff is terminated because the context has been canceled.

Which issue(s) this PR fixes:

N/A

Checklist

  • Tests updated
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@pracucci
Copy link
Contributor Author

pracucci commented Jul 3, 2024

I just realised we require golang 1.20 in dskit. Damn this repo lives in the past!

Copy link
Collaborator

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

LGTM - except I guess we need to increase the minimum Go version to 1.21. Does anything hinder us from doing so?

@pstibrany
Copy link
Member

If there are objections to this change, then an alternative may be adding another function Backoff.ErrCause() which is like Err() but returns the context cause instead.

This seems like safer option to me, to avoid breaking changes to clients using Backoff today.

@simonswine
Copy link
Contributor

If there are objections to this change, then an alternative may be adding another function Backoff.ErrCause() which is like Err() but returns the context cause instead.

This seems like safer option to me, to avoid breaking changes to clients using Backoff today.

I would agree with @pstibrany: A subtle breaking change like this will not come up when randomly upgrading the dskit package, and not too sure that everyone fully check the diff between go mod updates.

I could also think of something that would at least make the errors.Is still work by wrapping the cause: https://go.dev/play/p/tT6qg_rnDxd, but still prefer the simplicity of a separate method.

@pracucci
Copy link
Contributor Author

pracucci commented Jul 3, 2024

Thanks for the feedback. Switching to draft cause it requires golang 1.21 anyway, so we would need to upgrade first.

@pracucci pracucci marked this pull request as draft July 3, 2024 16:14
@pracucci pracucci changed the title Backoff: change Err() to return the context cause when available Add Backoff.ErrCause() Jul 3, 2024
@pracucci
Copy link
Contributor Author

pracucci commented Jul 3, 2024

Thanks for the feedback people. I've added Backoff.ErrCause() instead, so that this PR is not a breaking change. Still blocked on #540 tho.

@pracucci pracucci force-pushed the add-context-cause-support-to-backoff branch from 9b0cfc8 to 2ee2549 Compare July 4, 2024 08:09
Signed-off-by: Marco Pracucci <[email protected]>
@pracucci pracucci marked this pull request as ready for review July 4, 2024 08:10
@pracucci
Copy link
Contributor Author

pracucci commented Jul 4, 2024

Still blocked on #540 tho.

I merged #540

Copy link
Contributor

@simonswine simonswine left a comment

Choose a reason for hiding this comment

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

LGTM

@pracucci pracucci merged commit 97b2aa9 into main Jul 4, 2024
6 checks passed
@pracucci pracucci deleted the add-context-cause-support-to-backoff branch July 4, 2024 11:38
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.

4 participants