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

Uprade gobreaker version to 0.5.0 #640

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

sagynbek
Copy link

@sagynbek sagynbek commented Jan 16, 2024

💸 TL;DR

Update gobreaker version from 0.4.1 to 0.5.0, which adds IsSuccess to Config

📜 Details

We want to be able to customize the default behavior of IsSuccess, which defaults to check: if err is nil then it's successful, if err is non-nill then it's error
Outcome: By exposing IsSuccess to user, user will be able to make more thorough check

🧪 Testing Steps / Validation

✅ Checks

  • CI tests (if present) are passing
  • Adheres to code style for repo
  • Contributor License Agreement (CLA) completed if not a Reddit employee

@sagynbek sagynbek requested a review from fishy January 16, 2024 17:10
@sagynbek sagynbek marked this pull request as ready for review January 16, 2024 17:10
@sagynbek sagynbek requested a review from a team as a code owner January 16, 2024 17:10
@sagynbek sagynbek requested review from kylelemons and konradreiche and removed request for a team January 16, 2024 17:10
Copy link
Member

@fishy fishy left a comment

Choose a reason for hiding this comment

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

The majority of our code using breaker is like this:

resp, err := breaker.Execut(func() (any, error) {
  resp, err := actualCall(...)
  return resp, err
})

which means the feature IsSuccessful provided can easily be achieved via:

var resp respType
var err error
breaker.Execut(func() (any, error) {
  resp, err = actualCall(...)
  if myIsSuccessful(err) {
    return resp, nil
  }
  return resp, err
})

so I don't think there's any urgency to merge this (as none of the use cases are actually blocked by this), I'd prefer to wait for some services actually try this to see how useful it is before proceeding.

breakerbp/failure_ratio.go Outdated Show resolved Hide resolved
Co-authored-by: Yuxuan 'fishy' Wang <[email protected]>
@sagynbek
Copy link
Author

That's good workaround, it could be useful for this case:

We get validation error, we don't want it to increase error count of Circuit breaker, but still want original err to exit function.

Counter argument might be, if it's validation error then probably resp is nil and we could use that to check? That would be right, but in general IsSuccess looks useful overall )) It's not urgent to merge, but might be included in next release?
cc: @fishy

@kylelemons
Copy link
Contributor

Have you tested this version in production yet?

@konradreiche konradreiche requested review from fishy and removed request for konradreiche January 17, 2024 16:57
@sagynbek
Copy link
Author

Have you tested this version in production yet?

Hey @kylelemons , how can I test in production? Just bump version, and test it on staging?

Copy link
Contributor

@kylelemons kylelemons left a comment

Choose a reason for hiding this comment

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

You can use go get to update the dependency in your own application, you do not need to update it in baseplate in order for you to update.

As was mentioned above, it also doesn't seem like IsSuccessful is needed given the usual patterns of gobreaker, in which case you wouldn't even need to make your own copy of NewFailureRatioBreaker to test out the ability, but you could copy that function into your own repo it looks like to validate IsSuccessful is working as you expect.

And by production, I do mean production, not staging :D.

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

Successfully merging this pull request may close these issues.

3 participants