Skip to content

Conversation

@bobpaulin
Copy link
Contributor

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 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

@davsclaus
Copy link
Contributor

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.

@bobpaulin
Copy link
Contributor Author

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 would 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.

@davsclaus
Copy link
Contributor

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.

@davsclaus
Copy link
Contributor

Okay I think its better to use this PR to fix the current behaviour/regression.

Working on merging this manually so closing this PR

@davsclaus davsclaus closed this Feb 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants