Spring Integration
  1. Spring Integration
  2. INT-2649

Add transaction management support for DelayHandler's schedule tasks

    Details

      Issue Links

        Activity

        Hide
        Gary Russell added a comment -

        Hi Artem,

        We were just talking about this; we were thinking maybe just transactional is ok for now, and we can add advice chain later, if someone has a use case. However, if you have it already, go ahead and issue a PR and we can take a look.

        Show
        Gary Russell added a comment - Hi Artem, We were just talking about this; we were thinking maybe just transactional is ok for now, and we can add advice chain later, if someone has a use case. However, if you have it already, go ahead and issue a PR and we can take a look.
        Hide
        Artem Bilan added a comment -

        Thank you, guys, for your attention.
        Issue PR is here: https://github.com/SpringSource/spring-integration/pull/546

        Show
        Artem Bilan added a comment - Thank you, guys, for your attention. Issue PR is here: https://github.com/SpringSource/spring-integration/pull/546
        Hide
        Artem Bilan added a comment -

        Hi, Gary!
        Thank you for merging my PR, but I was changing my mind and was going to rework dalayer's 'advice-chain' ability according to your 'request-handler-advice-chain'.
        And also I was planing to remove <transactional> as you didn't provide it in the 'request-handler-advice-chain'.
        However If you think that my work OK with some your polishing, let it be.

        Now a little question: Does it make sense to mention this changes in the Manual and the Migration Guide?

        And my apologies that I stop contributing intensively.

        Show
        Artem Bilan added a comment - Hi, Gary! Thank you for merging my PR, but I was changing my mind and was going to rework dalayer's 'advice-chain' ability according to your 'request-handler-advice-chain'. And also I was planing to remove <transactional> as you didn't provide it in the 'request-handler-advice-chain'. However If you think that my work OK with some your polishing, let it be. Now a little question: Does it make sense to mention this changes in the Manual and the Migration Guide? And my apologies that I stop contributing intensively.
        Hide
        Gary Russell added a comment -

        Hi Artem,

        No; providing the ability to making the delayer's "dispatcher" thread transactional is completely orthogonal to the <request-handler-advice-chain/> implementation. If I had added that feature to the delayer, it would apply to the storing of the delayed message only, and nothing to do with dispatching the delayed message later.

        Also, the <request-handler-advice-chain /> is not intended to make the handleRequestMessage() transactional, it is to add advices such as expression evaluating (after success or failure) against the inbound message - such as to remove or move a file payload; circuit breaker advice; retry advice. Of course, someone could add a tx advice, but that is certainly not the intent of the feature.

        Note: I did not add <request-handler-advice-chain/> to the delayer; at the time, I didn't think it made sense. I suppose one could make an argument that a retry advice might be useful when using, say, a JDBC message store. If you think it's important, open a JIRA (or we can wait until someone asks for it). Note that, while merging, I changed your property to 'delayedAdviceChain' to avoid collision with the ARPMH superclass property (which is not currently exposed on the delay handler).

        Show
        Gary Russell added a comment - Hi Artem, No; providing the ability to making the delayer's "dispatcher" thread transactional is completely orthogonal to the <request-handler-advice-chain/> implementation. If I had added that feature to the delayer, it would apply to the storing of the delayed message only, and nothing to do with dispatching the delayed message later. Also, the <request-handler-advice-chain /> is not intended to make the handleRequestMessage() transactional, it is to add advices such as expression evaluating (after success or failure) against the inbound message - such as to remove or move a file payload; circuit breaker advice; retry advice. Of course, someone could add a tx advice, but that is certainly not the intent of the feature. Note: I did not add <request-handler-advice-chain/> to the delayer; at the time, I didn't think it made sense. I suppose one could make an argument that a retry advice might be useful when using, say, a JDBC message store. If you think it's important, open a JIRA (or we can wait until someone asks for it). Note that, while merging, I changed your property to 'delayedAdviceChain' to avoid collision with the ARPMH superclass property (which is not currently exposed on the delay handler).
        Hide
        Artem Bilan added a comment -

        OK, Gary.
        Thank you very much for your clarification.

        I agree with you that now <request-handler-advice-chain/> doesn't make sense to DelayHandler#handleRequestMessage().
        But who know what people will come up... Time will tell.

        Nevertheless we should keep in mind: now we "advice" the relaese message thread, but the last one invokes handleRequestMessage() in the end. And people may be a bit confused that message-flow has different behavior when Message is delayed and when not.

        Show
        Artem Bilan added a comment - OK, Gary. Thank you very much for your clarification. I agree with you that now <request-handler-advice-chain/> doesn't make sense to DelayHandler#handleRequestMessage() . But who know what people will come up... Time will tell. Nevertheless we should keep in mind: now we "advice" the relaese message thread, but the last one invokes handleRequestMessage() in the end. And people may be a bit confused that message-flow has different behavior when Message is delayed and when not.

          People

          • Assignee:
            Artem Bilan
            Reporter:
            Artem Bilan
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 0.5d Original Estimate - 0.5d
              0.5d
              Remaining:
              Remaining Estimate - 0d
              0d
              Logged:
              Time Spent - 0.75d
              0.75d