search

awolden / brakes

Hystrix compliant Node.js Circuit Breaker Library
MIT License
300 stars 35 forks source link

Add `isFailure` option #58

Closed SimenB closed 7 years ago

SimenB commented 7 years ago

This copies the idea (and description) from Levee.

https://github.com/krakenjs/levee#isfailure

awolden commented 7 years ago

@SimenB This looks like a solid addition. I see no issue with releasing this as a minor version upgrade. I will merge and publish new version @ 2.4.0.

SimenB commented 7 years ago

@awolden Hmm, it probably shouldn't reject at all. Looking at Levee's implementation:

https://github.com/krakenjs/levee/blob/c8558d518a1900639cd0faca5e13229926cc84fb/lib/breaker.js#L104-L122

I can update tomorrow

awolden commented 7 years ago

It would seem strange to have it not reject. If the isFailure() function determines the error is not critical, it resolves the request? What would it return in the resolution?

Edit: Close and Comment button is way too close to the comment button. Reopened accidently closed PR.

SimenB commented 7 years ago

It would resolve with null I suppose By providing isFailure, and having it return false you explicitly say it's not a failure, thus it shouldn't reject.

If a fallback is defined resolve with that, otherwise just resolve?

SimenB commented 7 years ago

@awolden Thoughts on ^? I can update tomorrow if you agree

awolden commented 7 years ago

I think it makes sense as written. Levee still triggers the callback with the err as the first argument, essentially rejecting it. I don't think this requires any additional changes.

SimenB commented 7 years ago

Hmmm, true. Merge away, then!