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

feat: Client Circuit Breaker #930

Merged
merged 1 commit into from
Dec 31, 2024
Merged

Conversation

segevda
Copy link
Contributor

@segevda segevda commented Dec 24, 2024

Adds a circuit breaker to resty.Client which disables requests for a specified period of time upon multiple failures.
Default behavior is counting 5xx errors, but users can implement their own policies.

Resolves #448

Copy link

codecov bot commented Dec 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.81%. Comparing base (f2a67d3) to head (69e85d9).
Report is 1 commits behind head on v3.

Additional details and impacted files
@@           Coverage Diff           @@
##               v3     #930   +/-   ##
=======================================
  Coverage   99.81%   99.81%           
=======================================
  Files          16       17    +1     
  Lines        3695     3781   +86     
=======================================
+ Hits         3688     3774   +86     
  Misses          5        5           
  Partials        2        2           
Flag Coverage Δ
unittests 99.81% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@segevda segevda force-pushed the feat_circuit_breaker branch from fb9decd to f0d6169 Compare December 25, 2024 07:55
breaker.go Outdated Show resolved Hide resolved
breaker.go Outdated Show resolved Hide resolved
client.go Show resolved Hide resolved
Copy link
Member

@jeevatkm jeevatkm left a comment

Choose a reason for hiding this comment

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

@segevda - Thanks for the PR. Please incorporate the comments and suggestions.

@ccoVeille - Thanks for your review and input.

breaker.go Outdated Show resolved Hide resolved
breaker.go Outdated Show resolved Hide resolved
breaker.go Outdated Show resolved Hide resolved
breaker.go Outdated Show resolved Hide resolved
breaker.go Outdated Show resolved Hide resolved
client.go Show resolved Hide resolved
client.go Show resolved Hide resolved
client.go Outdated Show resolved Hide resolved
breaker.go Outdated Show resolved Hide resolved
breaker.go Outdated Show resolved Hide resolved
@jeevatkm jeevatkm added feature v3 For resty v3 labels Dec 29, 2024
@jeevatkm jeevatkm added this to the v3.0.0 Milestone milestone Dec 29, 2024
@segevda segevda force-pushed the feat_circuit_breaker branch 3 times, most recently from f1d00de to 7884ad5 Compare December 30, 2024 08:57
@segevda
Copy link
Contributor Author

segevda commented Dec 30, 2024

@ccoVeille @jeevatkm thanks for the review!

circuit_breaker.go Outdated Show resolved Hide resolved
circuit_breaker.go Outdated Show resolved Hide resolved
circuit_breaker.go Outdated Show resolved Hide resolved
circuit_breaker.go Outdated Show resolved Hide resolved
circuit_breaker.go Outdated Show resolved Hide resolved
circuit_breaker.go Outdated Show resolved Hide resolved
circuit_breaker.go Outdated Show resolved Hide resolved
circuit_breaker.go Outdated Show resolved Hide resolved
circuit_breaker.go Outdated Show resolved Hide resolved
circuit_breaker.go Outdated Show resolved Hide resolved
client.go Show resolved Hide resolved
client_test.go Outdated Show resolved Hide resolved
@segevda segevda force-pushed the feat_circuit_breaker branch 2 times, most recently from 989054a to 658a3e8 Compare December 31, 2024 08:21
@jeevatkm
Copy link
Member

@segevda Could you please remove the go.mod and go.sum changes from this PR?

@segevda segevda force-pushed the feat_circuit_breaker branch from 658a3e8 to 87b73cf Compare December 31, 2024 16:45
@segevda
Copy link
Contributor Author

segevda commented Dec 31, 2024

@segevda Could you please remove the go.mod and go.sum changes from this PR?

oops. removed

@ccoVeille
Copy link

ccoVeille commented Dec 31, 2024

I feel like I contributed to this PR 😅

Adding a Co-authored-by tag to your commit would be appreciated then 😬

Co-authored-by: ccoVeille <[email protected]>

https://docs.github.com/articles/creating-a-commit-with-multiple-authors

@jeevatkm
Copy link
Member

I feel like I contributed to this PR 😅

Adding a Co-authored-by tag to your commit would be appreciated then 😬

Co-authored-by: ccoVeille <[email protected]>

https://docs.github.com/articles/creating-a-commit-with-multiple-authors

@ccoVeille Certainly, you have contributed via Reviewing PR. This is the first time I received this kind of request. I believe adding a reviewer tag is appropriate as follows -

Reviewed-by: ccoVeille <[email protected]>

What do you think?

@jeevatkm jeevatkm requested review from ccoVeille and jeevatkm and removed request for ccoVeille December 31, 2024 18:35
@ccoVeille
Copy link

I feel like I contributed to this PR 😅

Adding a Co-authored-by tag to your commit would be appreciated then 😬

Co-authored-by: ccoVeille <[email protected]>

https://docs.github.com/articles/creating-a-commit-with-multiple-authors

@ccoVeille Certainly, you have contributed via Reviewing PR.
This is the first time I received this kind of request.

GitHub supports co-authored-by natively when the owner or author uses commit feature on the suggestion provided by a contributor. So you may already have commits with such tags.

I believe adding a reviewer tag is appropriate as follows -

Reviewed-by: ccoVeille <[email protected]>

What do you think?

Yes, sure. Reviewed-By is less used but why not.

This tag is automatically added to merge commit on GitLab if the project is configured to support it via the templating system.

I don't know if it can be configured on GitHub

@ccoVeille
Copy link

ccoVeille commented Dec 31, 2024

So you may already have commits with such tags.

Confirmed, I found one pretty easily: 9097b96

@ccoVeille
Copy link

ccoVeille commented Dec 31, 2024

This is the first time I received this kind of request.

About why I asked, there are multiple reasons:

  • I did contribute to the PR
  • I like to spread the knowledge, and here I could share the idea
  • code reviewing is important and owner/author apply code changes within their IDE, attributing themselves the idea
  • visibility of my contributions to OSS project
  • vanity (haha)

@segevda segevda force-pushed the feat_circuit_breaker branch from 87b73cf to ab9dda9 Compare December 31, 2024 19:41
@segevda
Copy link
Contributor Author

segevda commented Dec 31, 2024

@jeevatkm @ccoVeille squashed the commits with a more conventional commit msg and added the Reviewed-by trailer as agreed

@ccoVeille
Copy link

Thanks. My request was to have a co-authored-by tag.

@jeevatkm agreed and asked to add also the reviewed-by tag.

You squashed and used the reviewed-by tag.

But now, I'm not getting the tag I asked for 😅

Can I also get the co-authored-by one? Haha

@jeevatkm
Copy link
Member

@segevda, I just reviewed the code suggestion conversation history on this PR. @ccoVeille suggested code snippets, which GitHub typically considers as co-authors.

Can you please remove the reviewed-by tag and add the co-authored-by tag? Many thanks for considering my request.

Co-authored-by: ccoVeille <[email protected]>

@ccoVeille, if you're interested in contributing further to the documentation examples of the upcoming Resty v3 release, let me know, and I will share the line items.

@segevda segevda force-pushed the feat_circuit_breaker branch from ab9dda9 to 69e85d9 Compare December 31, 2024 21:04
@segevda
Copy link
Contributor Author

segevda commented Dec 31, 2024

Can you please remove the reviewed-by tag and add the co-authored-by tag? Many thanks for considering my request.

Done 🙂

@ccoVeille
Copy link

ccoVeille commented Dec 31, 2024

I could help maybe yes. I can easily review and provide feedbacks about PR. I can do it from my couch, as I do now. Coding changes requires more efforts 😅

Copy link
Member

@jeevatkm jeevatkm left a comment

Choose a reason for hiding this comment

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

Thanks, @segevda, for incorporating the review comments and the request.

@jeevatkm
Copy link
Member

I could help maybe yes. I can easily review and provide feedbacks about PR. I can do it from my couch, as I do now. Coding chances requires more efforts 😅

@ccoVeille I meant creating a new example in documentation, not PR feedback. Of course, you could chime in when you see a PR.

@jeevatkm jeevatkm merged commit cf25584 into go-resty:v3 Dec 31, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature v3 For resty v3
Development

Successfully merging this pull request may close these issues.

3 participants