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

Cleaner error propagation in hystrix core v2 #1478

Closed
mNantern opened this issue Feb 14, 2017 · 7 comments
Closed

Cleaner error propagation in hystrix core v2 #1478

mNantern opened this issue Feb 14, 2017 · 7 comments
Labels

Comments

@mNantern
Copy link

I have exactly the same issue as in #1414 (fix works great btw, thx!) but for HystrixBadRequestException.

I tried to implement a HystrixCommandExecutionHook but all the methods are executed before decomposeException so I don't know how to make it work.

Any idea ?

Thank you !

@mattrjacobs
Copy link
Contributor

I'm not sure I follow. HystrixRuntimeException is generated by the Hystrix execution mechanism. In that case, having the option to wrap with or without it is reasonable. HystrixBadRequestException is never added by Hystrix, only by user code. So if you don't want callers to get HystrixBadRequestException, then you should de line to use it in your code.

@mNantern
Copy link
Author

My use case is pretty simple:

my HTTP clients are throwing functional exceptions that are catch in different places in my code. I don't want Hystrix to count some of the exception as server error (for example: bad parameters) and so I wrapped them in HystrixBadRequestException.
But now I'm forced to change all my catch to check for HystrixBadRequestException instead of my functional exceptions.

A possible workaround would be to check for the same interface (ExceptionNotWrappedByHystrix) in order to unwrap HystrixBadRequestException too.

What do you think ?

@mattrjacobs
Copy link
Contributor

OK, got it, thanks for clarifying - that does make sense to me now. For next steps, I can imagine the following implementations:

  • User can wrap business exception with HystrixBadRequestException and ExceptionNotWrappedByHystrix. Hystrix knows both should get unwrapped
  • We invent a new exception type: HystrixUnwrappedBadRequestException that has the proper semantics w.r.t. circuit-breaking but is handled by not propagating this Hystrix-internal type
  • We add configuration that decides whether to drop a HystrixBadRequestException or propagate it.

Any thoughts on which of these makes sense from your perspective?

@mNantern
Copy link
Author

Clearly the first one. I really like the fact that you can just add to your exception an implements ExceptionNotWrappedByHystrix and that's it everything works as expected !

@mattrjacobs
Copy link
Contributor

My schedule is jammed at the moment. Would you like to submit a PR with an implementation of this?

@mNantern
Copy link
Author

mNantern commented Mar 9, 2017

Sure !

I will change the method decomposeException in AbstractCommand to return the original exception when it is wrapped with HystrixBadRequestException and ExceptionNotWrappedByHystrix.

I don't know Hystrix very well but does it sound good for you ?

@mukteshkrmishra
Copy link

My :+1 for above approach. HowyI would like to have a look on it once ✅

mattrjacobs added a commit that referenced this issue Mar 24, 2017
Fixes #1478 : allow cleaner error propagation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants