You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@camel.apache.org by GitBox <gi...@apache.org> on 2021/01/26 22:21:59 UTC

[GitHub] [camel] bobpaulin opened a new pull request #4938: CAMEL-16083: Route Scoped OnCompletion After Consumer routeId check.

bobpaulin opened a new pull request #4938:
URL: https://github.com/apache/camel/pull/4938

Hi this is a suggested fix to CAMEL-16083 to support After Consumer mode onCompletion definitions in routeScope. Open to other suggestions.

The testOnCompletionInSub is the usecase I'm trying to solve for. The other unit tests were just other regressions I was concerned about.

- [ x] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/CAMEL) filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
- [x ] Each commit in the pull request should have a meaningful subject line and body.
- [ x] If you're unsure, you can format the pull request title like `[CAMEL-XXX] Fixes bug in camel-file component`, where you replace `CAMEL-XXX` with the appropriate JIRA issue.
- [x ] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [ x] Run `mvn clean install -Psourcecheck` in your module with source check enabled to make sure basic checks pass and there are no checkstyle violations. A more thorough check will be performed on your pull request automatically.
Below are the contribution guidelines:
https://github.com/apache/camel/blob/master/CONTRIBUTING.md

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [camel] davsclaus commented on pull request #4938: CAMEL-16083: Route Scoped OnCompletion After Consumer routeId check.

Posted by GitBox <gi...@apache.org>.

davsclaus commented on pull request #4938:
URL: https://github.com/apache/camel/pull/4938#issuecomment-771722558

Yeah but the onCompletion in the DSL was never intended to support triggering events at various routes during routing.
However the DSL is so open that it appears you can do that.

It was designed as a single action after the exchange was complete (on completion). And that you could specify that either as a global rule (context scoped), or override that and specify on the route. However the situation with sub routes having on completions wasn't taken well into the design. Nor that some users would specify 2+ on completions on a route, which meant that only the last would be used.

So I regret implementing this feature soo poorly.

However the real use-case it was solving was to do something around the consumer when the message is complete. However the point the action is triggered needed to have 2 modes (after some time) and hence we added before vs after consumer (the name may be poorly named).

So all together there are now a permutation of ways that on completion can be specifyied in the route DSL, and on global level, and then with 2 modes, then the end user dont get a great understanding what would happen.

Also when on completion is part of the DSL it becomes too easy to use, and therefore also mis-use for wrong use-cases.

There are other ways with event listeners, route policy / route policy factory that separates this better from the DSL.

With this PR then it opens up the possibilty to take in account onCompletions from sub routes.

Need a bit more thinking.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [camel] davsclaus commented on pull request #4938: CAMEL-16083: Route Scoped OnCompletion After Consumer routeId check.

Posted by GitBox <gi...@apache.org>.

davsclaus commented on pull request #4938:
URL: https://github.com/apache/camel/pull/4938#issuecomment-773126274


   Okay I think its better to use this PR to fix the current behaviour/regression.
   
   Working on merging this manually so closing this PR 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [camel] bobpaulin commented on pull request #4938: CAMEL-16083: Route Scoped OnCompletion After Consumer routeId check.

Posted by GitBox <gi...@apache.org>.

bobpaulin commented on pull request #4938:
URL: https://github.com/apache/camel/pull/4938#issuecomment-771066135

Agree and Thank you @davsclaus . I think allowing the onComplete code in After Consumer Mode in the sub-routes is a double edged sword. On one hand I can define the onComplete once in the sub-route and attach it to unlimited calling consumers. On the other hand coding this way disconnects the definition from where the code is going to be executed which could be confusing. The usecase I'm dealing with from code implemented on Camel 2.x code involved an event we wanted to fire as an async action triggered by something happening in a sub-route without slowing down the response to the client. While there are other approaches we could use to get this behavior After Consumer seemed like a natural fit so we did it that way. The issue with the current implementation is that in After Consumer Mode the definition must be done in the consumer route directly (for example a rest definition) or it will never fire. If this is something we're trying to make camel users adhere to then it wou
ld be good to get an exception or perhaps even defaulting sub route onCompletion code to Before Consumer Mode.

Both involve behavior changes which may or may not be acceptable. Which is why I've offered a PR proposal to try to allow some compatibility which feels a bit heavy to get it to work. However without access to the Route object in the onComplete(Exchange) method in the OnCompletion processor it seemed like the only way to ensure the After Consumer onComplete fired was to track the route id in the Exchange itself.

Thanks again for looking at this so quickly. Happy to bring this discussion on list if more usecases might help in determining a path forward.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [camel] davsclaus closed pull request #4938: CAMEL-16083: Route Scoped OnCompletion After Consumer routeId check.

Posted by GitBox <gi...@apache.org>.

davsclaus closed pull request #4938:
URL: https://github.com/apache/camel/pull/4938


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [camel] davsclaus closed pull request #4938: CAMEL-16083: Route Scoped OnCompletion After Consumer routeId check.

Posted by GitBox <gi...@apache.org>.

davsclaus closed pull request #4938:
URL: https://github.com/apache/camel/pull/4938


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [camel] davsclaus commented on pull request #4938: CAMEL-16083: Route Scoped OnCompletion After Consumer routeId check.

Posted by GitBox <gi...@apache.org>.

davsclaus commented on pull request #4938:
URL: https://github.com/apache/camel/pull/4938#issuecomment-771015712


   Thanks for the PR and attempt to help with this.
   
   Remind me tomorrow to look into this. Its both wrong and correct to have on completions firing for sub routes. It was intended only for when the exchange was done and not mid-stream. 
   
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [camel] davsclaus commented on pull request #4938: CAMEL-16083: Route Scoped OnCompletion After Consumer routeId check.

Posted by GitBox <gi...@apache.org>.

davsclaus commented on pull request #4938:
URL: https://github.com/apache/camel/pull/4938#issuecomment-773126274


   Okay I think its better to use this PR to fix the current behaviour/regression.
   
   Working on merging this manually so closing this PR 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [camel] davsclaus commented on pull request #4938: CAMEL-16083: Route Scoped OnCompletion After Consumer routeId check.

Posted by GitBox <gi...@apache.org>.

davsclaus commented on pull request #4938:
URL: https://github.com/apache/camel/pull/4938#issuecomment-771722558

Yeah but the onCompletion in the DSL was never intended to support triggering events at various routes during routing.
However the DSL is so open that it appears you can do that.

So I regret implementing this feature soo poorly.

Also when on completion is part of the DSL it becomes too easy to use, and therefore also mis-use for wrong use-cases.

There are other ways with event listeners, route policy / route policy factory that separates this better from the DSL.

With this PR then it opens up the possibilty to take in account onCompletions from sub routes.

Need a bit more thinking.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org