-
Notifications
You must be signed in to change notification settings - Fork 78
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
base: master
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.
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.
Co-authored-by: Yuxuan 'fishy' Wang <[email protected]>
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 Counter argument might be, if it's validation error then probably |
Have you tested this version in production yet? |
Hey @kylelemons , how can I test in production? Just bump version, and test it on staging? |
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.
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.
💸 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, iferr
is non-nill then it's errorOutcome: By exposing
IsSuccess
to user, user will be able to make more thorough check🧪 Testing Steps / Validation
✅ Checks