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

Allow exceptions without HystrixRuntimeException wrapper #1414

Merged
merged 1 commit into from
Dec 19, 2016

Conversation

tbvh
Copy link

@tbvh tbvh commented Nov 6, 2016

Introduce NotWrappedByHystrix to mark Exceptions that should not be wrapped in HystrixRunetimeException.

Partial fix for #1283 .

@tbvh tbvh changed the title Allow exceptions without HystrixRuntimeException wrapper #1283 Allow exceptions without HystrixRuntimeException wrapper Nov 6, 2016
@mattrjacobs
Copy link
Contributor

Thanks for the PR @tbvh ! This looks like a good change to me. I'm trying to think through side-effects of this change.

A) Only users who opt into using this interface get a change in behavior - that's a good thing
B) At runtime, the effect of the change is that executions that return any Exception instance which implements ExceptionNotWrappedByHystrix gets the raw exception
C) These exceptions manifest as FAILURE in the FailureType enum and metrics

For C), would it make sense to have a new event type (UNWRAPPED_FAILURE) that would make it easier to distinguish this scenario?

Any other users in the community have thoughts on this change?

@Sunshow
Copy link

Sunshow commented Dec 13, 2016

So will this PR be merged to master?

@tbvh
Copy link
Author

tbvh commented Dec 14, 2016

I just posted #1283 (comment).

Just to clarify, I've introduced this as a partial fix, but it completely solves point 1 of issue #1283. In the comment I propose to drop point 2, which make this PR in fact a complete fix.

Makes me all the more curious to the merge status. :)

@mattrjacobs
Copy link
Contributor

A couple thumbs-up from the community and I agree with the change. I think it's fine not to special-case this for metrics. A failure of the wrapped work should be a FAILURE as far as metrics are concerned, whether the exception is passed back directly or now. If anyone disagrees with this, then I think that' fine to have in a follow-on PR.

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

Successfully merging this pull request may close these issues.

3 participants