Javanica: Option to send fallback exception to client instead of primary command #1344
Comments
|
Which version of Javanica you are using? |
|
|
|
@nytins as it was discussed before it's hard to justify importance of an exception thrown from fallback over one thrown from command. Some clients are interested only in command exception, having said that I think it should be configurable. |
|
@dmgcodevil here the importance of the fallback exception is that it is one of the ignored exceptions. Essentially, an ignored exception should get priority over other exceptions in the flow. |
|
Another thing I noticed is that the behavior is different when the fallback method is async.
The below code snippet throws But the below snippet throws |
|
Ok, for instance you have two rest services: primary and backup. If the call to primary fails because of invalid parameters then it's consistent if fallback will fail because of invalid parameters as well; it's consistent behaviour so it doesn't matter which exemption is returned to end user, either of them is fine. On the other hand if fallback fails because of different reason then it becomes tricky. I think that returning command exception is more correct because essentially it's root cause exception. |
|
But if you consider ignored exceptions as part of the regular application flow, then this unnecessarily breaks application. Take this example: My understanding is that ignored exceptions doesn't represent a system failure. They are part of the usual application flow. So by not propagating them to client hystrix is blocking information flow to the client. |
|
"ignored exceptions" shouldn't have any impact on making decision which exception to propogate to end user. I was reasoning about propagating exception from fallback to end user and I think it makes sense because an user can get an exception thrown from command by adding addition Throwable argument in fallback method signature so that it's user responsiblity to process command exception, i.e. it can be re-thrown or just logged. |
|
I agree with the point that command exceptions can be seen in the fallback method but fallback exceptions cant be seen by the client, so it makes sense to throw fallback exceptions. But I also think that ignored exceptions should also have impact on this decision making. But I'm happy if we just decide to proceed with propagating fallback exception :) |
|
I had another issue related to an exception being thrown within a fallback, which was completely ignored (#1313). I'm joining the conversation by invitation of @dmgcodevil. The solution for my case is much simpler than what @nytins needs. I don't need to propagate the exception thrown within the fallback, just know that something unexpected happened in the fallback instead of silently fail. So in my case, the best option is (apart than logging that exception as agreed in #1313) to add a new hook to But it would be more flexible to be able to choose the exception that needs to be thrown to the client. Since in @nytins case, the client doesn't need to know that the first server is down, because he is dealing with it in the fallback. I'm not very familiar with the framework so I cannot contribute with something very useful. |
|
cc @michaelcowan, @jorgefr, @nytins
class HytrixFlowException extends RuntimeException {
Throwable commandException;
Throwable fallbackException; // or list of exceptions in the case of multiple fallback methods
}
@HytrixCommand(fallbackMethod = "fallback")
User getById(String id) {
throw new CommandException("primary server fails");
}
User fallback(String id, Throwable commandException) {
throw new HytrixFlowException (commandException, new FallbackException("fallback server fails"));
}Or always propagate command exception and omit fallback one: User fallback(String id, Throwable commandException) {
try {
// make call to backup service
} catch (Exception e) {
// fallback has failed but we don't care, primary is guilty
throw commandException;
}
}Although, it should be feasible to implement something like to force javanica always propagating only command exception and hide fallback exception. |
|
Hi this is a big issue for us. Also the proposed solution from @michaelcowan does not seem appropriate as having it option driven. Why cant we have it throw unchecked If the command it is executing throws any exception in that case let it be unwrapped. This way people using aspect and spring do not need to declare checked exception thrown from Hystrix (when This seem to be working fine prior to 1.5.4 |
|
hi @hroongta your suggestion is the same as @56quarters mentioned in the separate issue #1381 . It sounds reasonable for me. I will create PR shortly (this week). NOTE: I'm going to implement this approach along with one about wrapping platform exceptions like guys, are you ok with this design (time box is 2 days to make a decision) ? I want to nail down this issue on this week. cc @nytins, @hroongta, @56quarters, @jorgefr, @mukteshkrmishra |
|
@dmgcodevil : I believe that would be the correct approach (#1344 (comment)), as it will give the ability to a user for seeing real exception (which is logical). Going ahead, if user wants to see command and fallback exception the he/she can go for CustomException route. 👍 from my side. |
|
@dmgcodevil I am also happy with this approach. |
|
@dmgcodevil Mentioned in #1381, this approach works for me 👍 |
|
BTW, I've noticed that |
|
@dmgcodevil Is this not the same as my original suggestion? If it is I have sketched the idea out here https://github.com/michaelcowan/Hystrix/tree/feature/optionally-raise-hystrix-runtime-exception |
|
It looks ok to me. thanks! |
|
@michaelcowan could you please create a PR if you already have something. |
|
I think this issue has been addressed by #1397. If I'm mistake, please re-open |
|
So I'm just now discovering this and thought I would add my 2 cents. While this is a step in the right direction it does not completely solve the issue. Think about a developer who is using the MVC pattern - the service facade layer of the application is where all of this hystrix "stuff" comes into play. The API of that layer is the contract between this layer & the layer above it (for a web application - some controller/restful API layer). This addition makes the caller of the service facade layer have to know specific details that Hystrix is involved by having to catch HystrixRuntimeException, look for a fallback exception (which is generally a FallbackInvocationException) and then have to read the cause from that. Why can't we simply throw whatever exception is thrown from the fallback? I have a post on StackOverflow which describes the scenario I'm trying to handle. http://stackoverflow.com/questions/40495259/capture-exception-thrown-by-hystrix-fallback |
|
Actually I was able to solve my use case using this approach. I posted my solution back on my stackoverflow post: http://stackoverflow.com/questions/40495259/capture-exception-thrown-by-hystrix-fallback/40529532#40529532 |
|
Actually it seems that this only works while the circuit is closed. Once the circuit is tripped open the HystrixRuntimeException gets propagated back up. |
|
@edeandrea Have you managed to make it work?
|
|
@maxromanovsky, @edeandrea could you create a separate iss for this ? |
|
@dmgcodevil @edeandrea @maxromanovsky do we have any update on this, I'm using latest version of hystrix cloud from spring boot 2.1.0.RELEASE, HystrixRuntimeException gets propagated back, which brings no benefits, how can we over come it? |
We are having an issue because javanica always sends the primary command exception to the client.
Sample code
What is happening
If the first service is down the first command throws
ConnectExceptionand falls back to fallback method.If the fallback method throws
ValidationExceptionthen the originalConnectExceptionis thrown back to client.This behaviour is undesired because its just a
ValidationExceptionand ideally we should be sending a message to client saying there is some validation exception.What will make it better
If fallback throws any of the ignored exceptions then propagate that exception to the client instead of the exception from the primary method.
The text was updated successfully, but these errors were encountered: