Hystrix Javanica not wrapping any exceptions

[smjdowling]

Steps to reproduce:
In a HystrixCommand throw a new exception

Expected:
Exceptions should be wrapped in HystrixRuntimeException or HystrixBadRequestException (if ignored).

Actual:
The exception isn't wrapped -- the thrown exception is propagated instead of a wrapped exception.

[smjdowling]

related to #1273

[mattrjacobs]

Is this in master, or in a released version of Hystrix?

[smjdowling]

@mattrjacobs in master after testing the changes from pr #1273. Thanks.

[jbojar]

@smjdowling Are you sure that exceptions should be wrapped if using HystrixCommandAspect from javanica? Unwrapping from HystrixRuntimeException was added in HystrixCommandAspect in commit 155d4e9 and unwrapping from HystrixBadRequestException even earlier in commit 59bbe9f.

[dmgcodevil]

hi, @smjdowling , @jbojar thanks for the answer, yes, I removed the logic to wrap original exceptions in corresponding hystrix exceptions, because throwing hystrix exceptions makes error handling awkward in spring framework. see iss993

[michaelcowan]

I understand the idea of making Javanica as transparent as possible, however personally I prefer to keep things more Hystrix like and not assume anything. Perhaps using an annotation argument to allow choosing if causes should be raised would be an idea?

What concerns me more than my preference though, is that I can't see how the tests can possibly be testing ignoreExceptions. If an exception is ignored, then expected is that exception. If an exception is not ignored, then expected used to be HystrixRuntimeException but is now that exception. There is no difference. (See BasicDefaultIgnoreExceptionsTest.java)

[dmgcodevil]

@michaelcowan , regarding ignored exceptions. Yes it's expected behaviour to get user exception regardless whether it's ignored or not. Throwing hystrix exceptions is pointless for most users. To test ignored exceptions you should check command metrics/events to verify that fallback wasn't triggered. It's easy to do. The whole point of javanica is making your code doesn't depend on hystrix platform so that you can change this lib in the future without changing your code. Throwing platform specific exceptions is kind of leaking abstraction.

[michaelcowan]

@dmgcodevil I totally see where you are coming from. It is much more elegant and hugely simplified for most users - its just that now this is the only way. For me personally Javanica was simply a convenient way to integrate Hystrix without having to hand roll lots of command classes.

How would you feel about something like this?

Were you suggesting to check the command events in a unit test? How would you go about this?

[dmgcodevil]

@michaelcowan I'll take a look and answer latter

[dmgcodevil]

Were you suggesting to check the command events in a unit test? How would you go about this?

something like this:

 HystrixInvokableInfo<?> command = HystrixRequestLog.getCurrentRequest()
                .getAllExecutedCommands().iterator().next();
        // confirm that command has failed
        assertTrue(command.getExecutionEvents().contains(HystrixEventType.FAILURE));
        // and that fallback was successful
        assertTrue(command.getExecutionEvents().contains(HystrixEventType.FALLBACK_SUCCESS));

@michaelcowan let's continue discussion here

[mattrjacobs]

I think this has been closed by PRs since this issue was raised. If there are still open questions, please re-open.