Camel
  1. Camel
  2. CAMEL-960

DeadLetterChannel - option to mark the exchange as failure handled and that its OK

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.4.0
    • Fix Version/s: 1.5.0
    • Component/s: camel-core
    • Labels:
      None

      Description

      Currently the DeadLetterChannel sets the orignal caused exception on the exchange after it has been failure handled. We should support somekind of option to enable/disable this feature. We could support:

      • option on the endpoint to set this for all exchanges
      • support a special header key that end-users can insert per exchange to be more dynamic
      • maybe methods on Exchange to set this more easily
      • maybe some refinements in isFailureHandled() to cater for this

      And we should consider use a better keyname in the DLC where it stores the original exception.

      See nabble:
      http://www.nabble.com/JMS-%2B-Fault-td19778503s22882.html

      We might need to push this for Camel 2.0 but I think it is a feature that end-users would need sooner.

      Any thoughts, please write here?

        Activity

        Hide
        Claus Ibsen added a comment -

        Closing all 1.5.0 issues

        Show
        Claus Ibsen added a comment - Closing all 1.5.0 issues
        Hide
        Claus Ibsen added a comment -

        Hadrian I think the fluent builder handled is needed on the DeadLetterChannelBuilder itself?

        Now you can only let it be handled if you use the onException stuff.

        Show
        Claus Ibsen added a comment - Hadrian I think the fluent builder handled is needed on the DeadLetterChannelBuilder itself? Now you can only let it be handled if you use the onException stuff.
        Hide
        Hadrian Zbarcea added a comment -

        Committed revision 703622.

        Show
        Hadrian Zbarcea added a comment - Committed revision 703622.
        Hide
        Claus Ibsen added a comment -

        +1

        Check out the DefaultExchange.isFailed() as we use this one to test if the exchange is OK or NOT.

        Show
        Claus Ibsen added a comment - +1 Check out the DefaultExchange.isFailed() as we use this one to test if the exchange is OK or NOT.
        Hide
        Hadrian Zbarcea added a comment -

        Yeah, of course I'll put one method that takes a boolean, but I'll overload with something more flexible/dynamic too.
        I think we nailed this one. Thanks! I'll get it done first thing in the morning and I'll issue the 1.5 rc.

        Show
        Hadrian Zbarcea added a comment - Yeah, of course I'll put one method that takes a boolean, but I'll overload with something more flexible/dynamic too. I think we nailed this one. Thanks! I'll get it done first thing in the morning and I'll issue the 1.5 rc.
        Hide
        Hadrian Zbarcea added a comment -

        @Claus, I think we agree on that.

        [..] alter the restoreExceptionOnExchange method in DeadLetterChannel, to NOT set the exception, or removing the FAILURE_HANDLED_PROPERTY property will lead to the same thing: at the end of the day we don't what the exception back in the exchange if failureHandled(true) was invoked during handling, right?

        I am just trying to avoid using extra headers. Maybe reset the value of FAILURE_HANDLED_PROPERTY to null, and then check that in restoreExceptionOnExchange(), not sure yet what works best, but the idea is to not put the exception back in the exchange. Agree?

        Show
        Hadrian Zbarcea added a comment - @Claus, I think we agree on that. [..] alter the restoreExceptionOnExchange method in DeadLetterChannel, to NOT set the exception, or removing the FAILURE_HANDLED_PROPERTY property will lead to the same thing: at the end of the day we don't what the exception back in the exchange if failureHandled(true) was invoked during handling, right? I am just trying to avoid using extra headers. Maybe reset the value of FAILURE_HANDLED_PROPERTY to null, and then check that in restoreExceptionOnExchange(), not sure yet what works best, but the idea is to not put the exception back in the exchange. Agree?
        Hide
        Claus Ibsen added a comment -

        PS: I should have logged on IRC , but I usually dont do this before leaving for work.

        Show
        Claus Ibsen added a comment - PS: I should have logged on IRC , but I usually dont do this before leaving for work.
        Hide
        Claus Ibsen added a comment -

        +1, just make sure the API also supports setting a boolean directly, not having to wrap it in a Expression such as constant(true) etc. that can be a bit confusing.

        Show
        Claus Ibsen added a comment - +1, just make sure the API also supports setting a boolean directly, not having to wrap it in a Expression such as constant(true) etc. that can be a bit confusing.
        Hide
        Claus Ibsen added a comment -

        @Hadrian

        if (processor != null) {
          data.failureProcessor = processor;
        }
        

        A good idea, however the default processor in DLC is a Logger that logs at ERROR, so the processor is by default != null.
        I don't think logging it at ERROR should be considered as handled. But yet again the default currently in Camel with DLC and TransctedExchanges etc. is up for a revision in Camel 2.0.

        With your idea then the catch-22 from above will work. Could you try the change and see how many unit tests breaks

        Show
        Claus Ibsen added a comment - @Hadrian if (processor != null ) { data.failureProcessor = processor; } A good idea, however the default processor in DLC is a Logger that logs at ERROR, so the processor is by default != null. I don't think logging it at ERROR should be considered as handled. But yet again the default currently in Camel with DLC and TransctedExchanges etc. is up for a revision in Camel 2.0. With your idea then the catch-22 from above will work. Could you try the change and see how many unit tests breaks
        Hide
        Hadrian Zbarcea added a comment -

        @Claus, I got your point. I agree, the default should be false.

        Again, the transform(...) is already possible. The only thing missing is the failureHandled() api, which could be actually implemented (I guess) to remove the FAILURE_HANDLED_PROPERTY property which contains the exception from the exchange. I'll probably use an expression there, not just a boolean. Piece of cake.

        I think this fully solves the problem. Do we agree on the solution?

        Show
        Hadrian Zbarcea added a comment - @Claus, I got your point. I agree, the default should be false. Again, the transform(...) is already possible. The only thing missing is the failureHandled() api, which could be actually implemented (I guess) to remove the FAILURE_HANDLED_PROPERTY property which contains the exception from the exchange. I'll probably use an expression there, not just a boolean. Piece of cake. I think this fully solves the problem. Do we agree on the solution?
        Hide
        Claus Ibsen added a comment - - edited

        @Hadrian

        No/yes its possible today, but we have a catch-22 situation where you have a route path using DLC and want it eg. to move a failued exchanges to a JMS queue.

        errorHandler(deadLetterChannel().to("jms:failed");
        from("jms:queue.a").to("bean:doSomething", "jms:queue.b");
        

        If you use transacted JMS then you can not handle the exchange currently in Camel. If bean:doSomething throws an Exception it is caught by the DLC that will try to failure handle it and move it to jms:failed. This is possible, but then it restoresTheOriginalException and then transaction handler in Camel will rethrow this and then the TX is marked for rollback.

        So what we want is to alter the restoreExceptionOnExchange method in DeadLetterChannel, to NOT set the exception. But of course it should be configurable when to do this

        Show
        Claus Ibsen added a comment - - edited @Hadrian No/yes its possible today, but we have a catch-22 situation where you have a route path using DLC and want it eg. to move a failued exchanges to a JMS queue. errorHandler(deadLetterChannel().to( "jms:failed" ); from( "jms:queue.a" ).to( "bean:doSomething" , "jms:queue.b" ); If you use transacted JMS then you can not handle the exchange currently in Camel. If bean:doSomething throws an Exception it is caught by the DLC that will try to failure handle it and move it to jms:failed . This is possible, but then it restoresTheOriginalException and then transaction handler in Camel will rethrow this and then the TX is marked for rollback. So what we want is to alter the restoreExceptionOnExchange method in DeadLetterChannel, to NOT set the exception. But of course it should be configurable when to do this
        Hide
        Claus Ibsen added a comment -

        What should the default be for failureHandled? Currently it is false all the time.

        So the caller will know that the exchange failed and what the caused exception was. If we set failureHandled=true as default then the caller will newer know this and not get a correct response either.

        What we want to support with this is that you can change this and say it is OK and also set another response if you like, something like this:

        onException(NoOrderException.class).maximumRedeliveries(0).failreHandled(true).transform(constant("No Order");
        

        And the caller would get the "No Order" text as a response instead of a NoOrderException.

        Show
        Claus Ibsen added a comment - What should the default be for failureHandled? Currently it is false all the time. So the caller will know that the exchange failed and what the caused exception was. If we set failureHandled=true as default then the caller will newer know this and not get a correct response either. What we want to support with this is that you can change this and say it is OK and also set another response if you like, something like this: onException(NoOrderException.class).maximumRedeliveries(0).failreHandled( true ).transform(constant( "No Order" ); And the caller would get the "No Order" text as a response instead of a NoOrderException.
        Hide
        Hadrian Zbarcea added a comment - - edited

        @Claus, but that is already possible today (with the exception of the failureHandled() api). What you added to my comment is that you want it explicit, not implicit. Correct? (I am on irc, btw)

        One more question. If you want it explicit what should the default be? If it's handled by onException() should we throw the RuntimeCamelException by default (if failureHandled(boolean) is not invoked) or consider it handled?

        RE: BTW: The fix should be in restoreExceptionOnExchange in DeadLetterChannel.
        Right, but i thought about not invoking restore... at all if this code above was executed (i.e. processor is not null) and there is no other exception:

        if (processor != null) {
          data.failureProcessor = processor;
        }
        
        Show
        Hadrian Zbarcea added a comment - - edited @Claus, but that is already possible today (with the exception of the failureHandled() api). What you added to my comment is that you want it explicit, not implicit. Correct? (I am on irc, btw) One more question. If you want it explicit what should the default be? If it's handled by onException() should we throw the RuntimeCamelException by default (if failureHandled(boolean) is not invoked) or consider it handled? RE: BTW: The fix should be in restoreExceptionOnExchange in DeadLetterChannel. Right, but i thought about not invoking restore... at all if this code above was executed (i.e. processor is not null) and there is no other exception: if (processor != null ) { data.failureProcessor = processor; }
        Hide
        Claus Ibsen added a comment - - edited

        Hadrian, yes the unit test is what we want, but I do think that this behavior should be configurable from

        • a new fluent build syntax to indicate its handled.
        • a special header on the message to indicate its handled (eg you can do some java coding in a processor and set that you handled it or not)
        • onException should not handle it by default (but interesting thought)
        • failureHandled fluent syntax also supports a predicate so you can use

        So something like a failureHandled fluent builder

        errorHandler(deadLetterChannel().maximumRedeliveries(3).failureHandled(true)
        

        Also on the onException that I think is a cool feature

        onException(IOException.class).maximumRedeliveries(0).failreHandled(true).to("mock:handled");
        

        And to let end users do some java coding of their own and set a special header to indicate handled or not

        onException(IOException.class).maximumRedeliveries(0).process(myFailureHandlerCode);
        
        public class MyFailureHandlerCode implements Processor {
        ...
           public void process(Exchange exchange) throws Exception {
            .. // do something
             // ah I can handle it so I set this special header
            exchange.getIn().setHeader("CamelFailureHandled", true);
        }
        

        And with the predicate for the fluent builder

        errorHandler(deadLetterChannel().maximumRedeliveries(3).failureHandled(header("CanIDoIt"))
        
        errorHandler(deadLetterChannel().maximumRedeliveries(3).failureHandled(bean(CanIHandleThis.class))
        

        Also supporting on the onException as well:

        onException(IOException.class).maximumRedeliveries(0).failreHandled(bean(CanIHandleThis.class).to("mock:handled");
        

        BTW: The fix should be in restoreExceptionOnExchange in DeadLetterChannel.

        Show
        Claus Ibsen added a comment - - edited Hadrian, yes the unit test is what we want, but I do think that this behavior should be configurable from a new fluent build syntax to indicate its handled. a special header on the message to indicate its handled (eg you can do some java coding in a processor and set that you handled it or not) onException should not handle it by default (but interesting thought) failureHandled fluent syntax also supports a predicate so you can use So something like a failureHandled fluent builder errorHandler(deadLetterChannel().maximumRedeliveries(3).failureHandled( true ) Also on the onException that I think is a cool feature onException(IOException.class).maximumRedeliveries(0).failreHandled( true ).to( "mock:handled" ); And to let end users do some java coding of their own and set a special header to indicate handled or not onException(IOException.class).maximumRedeliveries(0).process(myFailureHandlerCode); public class MyFailureHandlerCode implements Processor { ... public void process(Exchange exchange) throws Exception { .. // do something // ah I can handle it so I set this special header exchange.getIn().setHeader( "CamelFailureHandled" , true ); } And with the predicate for the fluent builder errorHandler(deadLetterChannel().maximumRedeliveries(3).failureHandled(header( "CanIDoIt" )) errorHandler(deadLetterChannel().maximumRedeliveries(3).failureHandled(bean(CanIHandleThis.class)) Also supporting on the onException as well: onException(IOException.class).maximumRedeliveries(0).failreHandled(bean(CanIHandleThis.class).to( "mock:handled" ); BTW: The fix should be in restoreExceptionOnExchange in DeadLetterChannel.
        Hide
        Hadrian Zbarcea added a comment - - edited

        I think I can get this fixed pretty quickly, but I am not sure I understand the issue well. There is fairly good set of tests in ExceptionBuilderTest.java. From what I understand your point is that if one specifies something like:

        onException(IOException.class).maximumRedeliveries(0).to("mock:handled");
        from("direct:a").to("mock:result");
        

        then the following code (which works today) is wrong:

        try {
          template.sendBody("direct:a", "Hello world");
          fail("Should have thrown a MyBusinessException");
        } catch (RuntimeCamelException e) {
          assertTrue(e.getCause() instanceof MyBusinessException);
          // expected
        }
        

        and RuntimeCamelException should not be thrown because the exception was actually handled in a satisfactory way. The test should only have:

          MockEndpoint result = getMockEndpoint("mock:result");
          result.expectedMessageCount(0);
          MockEndpoint handled = getMockEndpoint("mock:handled");
          handled.expectedMessageCount(1);
        
          template.sendBody("direct:a", "Hello world");
          // no exception, mock:result does not get a message, mock: handled gets one, no redelivery attempts
          result.assertIsSatisfied();
          handled.assertIsSatisfied();
        

        For other exceptions that are not specified in an onException statement, or if onException only modifies the redelivery policy, the behavior should stay the same, and the RuntimeCamelException is to be expected.

        Is my understanding correct?

        Show
        Hadrian Zbarcea added a comment - - edited I think I can get this fixed pretty quickly, but I am not sure I understand the issue well. There is fairly good set of tests in ExceptionBuilderTest.java. From what I understand your point is that if one specifies something like: onException(IOException.class).maximumRedeliveries(0).to( "mock:handled" ); from( "direct:a" ).to( "mock:result" ); then the following code (which works today) is wrong : try { template.sendBody( "direct:a" , "Hello world" ); fail( "Should have thrown a MyBusinessException" ); } catch (RuntimeCamelException e) { assertTrue(e.getCause() instanceof MyBusinessException); // expected } and RuntimeCamelException should not be thrown because the exception was actually handled in a satisfactory way. The test should only have: MockEndpoint result = getMockEndpoint( "mock:result" ); result.expectedMessageCount(0); MockEndpoint handled = getMockEndpoint( "mock:handled" ); handled.expectedMessageCount(1); template.sendBody( "direct:a" , "Hello world" ); // no exception, mock:result does not get a message, mock: handled gets one, no redelivery attempts result.assertIsSatisfied(); handled.assertIsSatisfied(); For other exceptions that are not specified in an onException statement, or if onException only modifies the redelivery policy, the behavior should stay the same, and the RuntimeCamelException is to be expected. Is my understanding correct?
        Hide
        Claus Ibsen added a comment -

        Hadrian did you start on this one? Its very important for 1.5? Otherwise I have time this weekend.

        Show
        Claus Ibsen added a comment - Hadrian did you start on this one? Its very important for 1.5? Otherwise I have time this weekend.
        Hide
        Claus Ibsen added a comment -
        Show
        Claus Ibsen added a comment - See this one as well: http://www.nabble.com/Exception-Handling-td19863662s22882.html

          People

          • Assignee:
            Hadrian Zbarcea
            Reporter:
            Claus Ibsen
          • Votes:
            4 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development