You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by "lburgazzoli (via GitHub)" <gi...@apache.org> on 2024/02/22 14:56:58 UTC
[PR] feat(camel-main): add options to let the shutdown strategy to wait or ignore inflight messages [camel]
lburgazzoli opened a new pull request, #13273:
URL: https://github.com/apache/camel/pull/13273
# Description
<!--
- Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
-->
# Target
- [ ] I checked that the commit is targeting the correct branch (note that Camel 3 uses `camel-3.x`, whereas Camel 4 uses the `main` branch)
# Tracking
- [ ] If this is a large change, bug fix, or code improvement, I checked there is a [JIRA issue](https://issues.apache.org/jira/browse/CAMEL) filed for the change (usually before you start working on it).
<!--
# *Note*: trivial changes like, typos, minor documentation fixes and other small items do not require a JIRA issue. In this case your pull request should address just this issue, without pulling in other changes.
-->
# Apache Camel coding standards and style
- [ ] I checked that each commit in the pull request has a meaningful subject line and body.
<!--
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.
-->
- [ ] I have run `mvn clean install -DskipTests` locally and I have committed all auto-generated changes
<!--
You can run the aforementioned command in your module so that the build auto-formats your code. This will also be verified as part of the checks and your PR may be rejected if if there are uncommited changes after running `mvn clean install -DskipTests`.
You can learn more about the contribution guidelines at https://github.com/apache/camel/blob/main/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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(camel-main): add options to let the shutdown strategy to wait or ignore inflight messages [camel]
Posted by "lburgazzoli (via GitHub)" <gi...@apache.org>.
lburgazzoli commented on PR #13273:
URL: https://github.com/apache/camel/pull/13273#issuecomment-1959630115
@davsclaus this is a draft work about having camel main be able to ignore or not inflight exchanges before shutting down the context. Not sure if this is the right approach as it seems that there is also some similar support in the `DefaultShutdownStrategy` so wonder where the best place to implement this feature would be
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(camel-main): add options to let the shutdown strategy to wait or ignore inflight messages [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #13273:
URL: https://github.com/apache/camel/pull/13273#issuecomment-1964441514
> mh, thinking a little bit more about this, any combination would probably be sub optimal for the camel-k POV, so I'm inclined to keep the implementation on the camel-k side and avoid to clutter camel-main
>
> @davsclaus @orpiske wdyt ?
okay that is of course welcome
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(camel-main): add options to let the shutdown strategy to wait or ignore inflight messages [camel]
Posted by "orpiske (via GitHub)" <gi...@apache.org>.
orpiske commented on PR #13273:
URL: https://github.com/apache/camel/pull/13273#issuecomment-1963954707
IMHO, I think an `enum` would be better ... something like `QuiescePolicy -> { INCLUSIVE, EXCLUSIVE }`
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(camel-main): add options to let the shutdown strategy to wait or ignore inflight messages [camel]
Posted by "lburgazzoli (via GitHub)" <gi...@apache.org>.
lburgazzoli commented on PR #13273:
URL: https://github.com/apache/camel/pull/13273#issuecomment-1964349200
mh, I'm not sure if this is gogin to work in the camel-k scenario as for example:
1. you set the max messages to 1
2. you set the max idle to 5 sec
Now, if an "child" exchange takes 10 second to complete, then the context would be shut down never the less.
If you set the max idle to i.e. 60 seconds, then the exchange won' be shut down till the 60 secs are gone.
I'm now thinking that given this is a specific camel-k behavior, this feature is likely better suited to be implemented in camel-k only ... @davsclaus @orpiske what do you think ?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(camel-main): add options to let the shutdown strategy to wait or ignore inflight messages [camel]
Posted by "lburgazzoli (via GitHub)" <gi...@apache.org>.
lburgazzoli commented on PR #13273:
URL: https://github.com/apache/camel/pull/13273#issuecomment-1963685495
> Yeah I can see the point that max does not have a inflight check, but that was also not its intention. But to say that after X message then shutdown. And during shutdown any inflight messages will be allowed to complete (within its timeout period).
>
> And on the other handle the maxIdle, should only shutdown if no new messages for X period and that no inflight messages is processing (i.e. Camel is doing nothing).
>
> So what you may ask for is to say max X and also being idle at the same time.
Do you think I should do something different ? like removing the options and combine max & idle so if they are turned on at the same time, then shutdown happens only when a number of messages have been processed and no activity in progress ?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(camel-main): add options to let the shutdown strategy to wait or ignore inflight messages [camel]
Posted by "lburgazzoli (via GitHub)" <gi...@apache.org>.
lburgazzoli commented on PR #13273:
URL: https://github.com/apache/camel/pull/13273#issuecomment-1963708431
> > > Yeah I can see the point that max does not have a inflight check, but that was also not its intention. But to say that after X message then shutdown. And during shutdown any inflight messages will be allowed to complete (within its timeout period).
> > > And on the other handle the maxIdle, should only shutdown if no new messages for X period and that no inflight messages is processing (i.e. Camel is doing nothing).
> > > So what you may ask for is to say max X and also being idle at the same time.
> >
> >
> > Do you think I should do something different ? like removing the options and combine max & idle so if they are turned on at the same time, then shutdown happens only when a number of messages have been processed and no activity in progress ?
>
> In a more general implementation detail: why not isolate this logic in a new, separate, interface `MainOnCompletionPolicy` or something like that and just add the new logic there (keeping both current and new behaviors as different implementations of the that interface? Sounds a lot less riskier than adding more cognitive/cyclomatic complexity to the code. IMHO.
>
Although I think this should be done at some point, I feel that as today the implementation is inconsistent as my understanding is that if you set bot max messages and idle time, then the max messages would win silently, @davsclaus am I right ?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(camel-main): add options to let the shutdown strategy to wait or ignore inflight messages [camel]
Posted by "lburgazzoli (via GitHub)" <gi...@apache.org>.
lburgazzoli commented on PR #13273:
URL: https://github.com/apache/camel/pull/13273#issuecomment-1962358545
If you don't see this generally applicable to camel-main, we can retain the behavior in camel-k without problems
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(camel-main): add options to let the shutdown strategy to wait or ignore inflight messages [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #13273:
URL: https://github.com/apache/camel/pull/13273#issuecomment-1963938991
> Do you think we need an option to turn on this behavior ?
Yes I had a walk with the dogs and thought the same. If we have an option to decide if its OR/AND then you can set both options and turn to AND and it should work for you as well. But what should such option be named ? And should it be a boolean or would an enum be better
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(camel-main): add options to let the shutdown strategy to wait or ignore inflight messages [camel]
Posted by "lburgazzoli (via GitHub)" <gi...@apache.org>.
lburgazzoli commented on PR #13273:
URL: https://github.com/apache/camel/pull/13273#issuecomment-1964420220
mh, thinking a little bit more about this, any combination would probably be sub optimal for the camel-k POV, so I'm inclined to keep the implementation on the camel-k side and avoid to clutter camel-main
@davsclaus @orpiske wdyt ?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(camel-main): add options to let the shutdown strategy to wait or ignore inflight messages [camel]
Posted by "lburgazzoli (via GitHub)" <gi...@apache.org>.
lburgazzoli commented on PR #13273:
URL: https://github.com/apache/camel/pull/13273#issuecomment-1964472212
> > mh, thinking a little bit more about this, any combination would probably be sub optimal for the camel-k POV, so I'm inclined to keep the implementation on the camel-k side and avoid to clutter camel-main
> > @davsclaus @orpiske wdyt ?
>
> Do you think it would be possible to make it work using the suggestion of abstracting it in a `MainOnCompletionPolicy` interface as I mentioned above? If yes, I think we could open a ticket and rework the code to allow you to implement the needed bits for Camel K without affecting the current behavior.
yes probably it would work
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(camel-main): add options to let the shutdown strategy to wait or ignore inflight messages [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #13273:
URL: https://github.com/apache/camel/pull/13273#issuecomment-1963609610
When this is done, then you need to add these 2 new options in camel-spring-boot project
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(camel-main): add options to let the shutdown strategy to wait or ignore inflight messages [camel]
Posted by "orpiske (via GitHub)" <gi...@apache.org>.
orpiske commented on code in PR #13273:
URL: https://github.com/apache/camel/pull/13273#discussion_r1499458392
##########
core/camel-main/src/main/java/org/apache/camel/main/MainDurationEventNotifier.java:
##########
@@ -110,10 +121,19 @@ protected void doNotify(CamelEvent event) {
if (complete) {
doneMessages.increment();
final int doneCount = doneMessages.intValue();
- final boolean result = doneCount >= maxMessages;
+ final int inflight = camelContext.getInflightRepository().size();
+
+ boolean result = doneCount >= maxMessages;
+ if (result && !maxMessagesIgnoreInflightExchanges) {
+ result = inflight == 0;
+ }
if (LOG.isTraceEnabled()) {
- LOG.trace("Duration max messages check {} >= {} -> {}", doneCount, maxMessages, result);
+ if (!maxMessagesIgnoreInflightExchanges) {
+ LOG.trace("Duration max messages check {}/{} >= {} -> {}", doneCount, inflight, maxMessages, result);
+ } else {
+ LOG.trace("Duration max messages check {} >= {} -> {}", doneCount, maxMessages, result);
+ }
Review Comment:
I'm wondering if we could have this in a way that retains the old logic. If you are processing a large batch of messages using the main, this part gets called quite a lot. So, this part was adjusted along with the performance fixes done around 4.0.0 development.
As it is now, by calling `camelContext.getInflightRepository().size();`, it is forcing the code to hit a path that is costlier. i.e.; the inflight repository instance is volatile, then the call itself may or many not be inlined as the JIT may not be able to determine its type, etc).
So, I think having 2 separate methods would be better here.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(camel-main): add options to let the shutdown strategy to wait or ignore inflight messages [camel]
Posted by "lburgazzoli (via GitHub)" <gi...@apache.org>.
lburgazzoli commented on PR #13273:
URL: https://github.com/apache/camel/pull/13273#issuecomment-1963890907
> > Although I think this should be done at some point, I feel that as today the implementation is inconsistent as my understanding is that if you set bot max messages and idle time, then the max messages would win silently, @davsclaus am I right ?
>
> They are independent, and works as an OR.
>
> However they don't work together as an AND, so you can say that they should both trigger before shutting down.
Do you think we need an option to turn on this behavior ?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(camel-main): add options to let the shutdown strategy to wait or ignore inflight messages [camel]
Posted by "lburgazzoli (via GitHub)" <gi...@apache.org>.
lburgazzoli commented on PR #13273:
URL: https://github.com/apache/camel/pull/13273#issuecomment-1964035551
Something like this ?
```
DurationStrategies { AND, OR }
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(camel-main): add options to let the shutdown strategy to wait or ignore inflight messages [camel]
Posted by "lburgazzoli (via GitHub)" <gi...@apache.org>.
lburgazzoli commented on PR #13273:
URL: https://github.com/apache/camel/pull/13273#issuecomment-1962350520
> This can lead to waiting forever as if the system keeps taking in new inflight messages, as this will never issue the shutdown strategy to begin shutting down. The shutdown strategy automatic let inflight messages complete (it has a timeout of 45 seconds). So with this in mind I think the current way should cover this.
>
> What is it that you see that dont work today?
This implementation is meant to explore how to move come logic that exists in the camel-k-runtime (the cron support) to camel so we can remove camel-k specific logic and have a generic solution.
What camel-k does is:
- to let Kubernetes handle job scheduling (leveraging the [Job](https://kubernetes.io/docs/concepts/workloads/controllers/job/) resource) instead of using the native one so the JVM does not requires to be always up and running
- to let Kubernetes natively handle [timeout/deadlines](https://kubernetes.io/docs/concepts/workloads/controllers/job/#job-termination-and-cleanup)
- to trigger the execution of the route, camel-k replace the scheduling component with a 1 shot timer event.
I knew about the shutdown timeout, but given the point above, the expectation is that the shutdown is not even triggered till all the task have been completed. So theoretically by setting a very high timeout you could achieve the same result, but you would see an entry in the log about the context being shutting down which may be confusing.
Still I don't really know if my implementation is correct or not, maybe we could think about a pluggable hook to let inject custom semantics.
>
> Also if something needs to be added then why are the 2 options false and true by default, can we not have just one option instead. Camel already have too many options and can confuse users.
I first had a single parameter, hover I noticed that the idle task, [take into account the inflight messages](https://github.com/apache/camel/blob/214ebc1f93790fe60d93d0c1e44fabdf51ae541f/core/camel-main/src/main/java/org/apache/camel/main/MainDurationEventNotifier.java#L248) before triggering the shutdown so to preserve the current behavior, I added a second option
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(camel-main): add options to let the shutdown strategy to wait or ignore inflight messages [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #13273:
URL: https://github.com/apache/camel/pull/13273#issuecomment-1963597873
Typo
Ingore -> Ignore
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(camel-main): add options to let the shutdown strategy to wait or ignore inflight messages [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #13273:
URL: https://github.com/apache/camel/pull/13273#issuecomment-1963830099
> Although I think this should be done at some point, I feel that as today the implementation is inconsistent as my understanding is that if you set bot max messages and idle time, then the max messages would win silently, @davsclaus am I right ?
They are independent, and works as an OR.
However they don't work together as an AND, so you can say that they should both trigger before shutting down.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(camel-main): add options to let the shutdown strategy to wait or ignore inflight messages [camel]
Posted by "orpiske (via GitHub)" <gi...@apache.org>.
orpiske commented on PR #13273:
URL: https://github.com/apache/camel/pull/13273#issuecomment-1963693654
> > Yeah I can see the point that max does not have a inflight check, but that was also not its intention. But to say that after X message then shutdown. And during shutdown any inflight messages will be allowed to complete (within its timeout period).
> > And on the other handle the maxIdle, should only shutdown if no new messages for X period and that no inflight messages is processing (i.e. Camel is doing nothing).
> > So what you may ask for is to say max X and also being idle at the same time.
>
> Do you think I should do something different ? like removing the options and combine max & idle so if they are turned on at the same time, then shutdown happens only when a number of messages have been processed and no activity in progress ?
In a more general implementation detail: why not isolate this logic in a new, separate, interface `MainOnCompletionPolicy` or something like that and just add the new logic there (keeping both current and new behaviors as different implementations of the that interface? Sounds a lot less riskier that adding more cognitive/cyclomatic complexity to the code. IMHO.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(camel-main): add options to let the shutdown strategy to wait or ignore inflight messages [camel]
Posted by "orpiske (via GitHub)" <gi...@apache.org>.
orpiske commented on code in PR #13273:
URL: https://github.com/apache/camel/pull/13273#discussion_r1500404647
##########
core/camel-main/src/main/java/org/apache/camel/main/MainDurationEventNotifier.java:
##########
@@ -110,10 +121,19 @@ protected void doNotify(CamelEvent event) {
if (complete) {
doneMessages.increment();
final int doneCount = doneMessages.intValue();
- final boolean result = doneCount >= maxMessages;
+ final int inflight = camelContext.getInflightRepository().size();
+
+ boolean result = doneCount >= maxMessages;
+ if (result && !maxMessagesIgnoreInflightExchanges) {
+ result = inflight == 0;
+ }
if (LOG.isTraceEnabled()) {
- LOG.trace("Duration max messages check {} >= {} -> {}", doneCount, maxMessages, result);
+ if (!maxMessagesIgnoreInflightExchanges) {
+ LOG.trace("Duration max messages check {}/{} >= {} -> {}", doneCount, inflight, maxMessages, result);
+ } else {
+ LOG.trace("Duration max messages check {} >= {} -> {}", doneCount, maxMessages, result);
+ }
Review Comment:
I think that would do the trick
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(camel-main): add options to let the shutdown strategy to wait or ignore inflight messages [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #13273:
URL: https://github.com/apache/camel/pull/13273#issuecomment-1962853355
Yeah lets give this some thinking on monday
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(camel-main): add options to let the shutdown strategy to wait or ignore inflight messages [camel]
Posted by "orpiske (via GitHub)" <gi...@apache.org>.
orpiske commented on PR #13273:
URL: https://github.com/apache/camel/pull/13273#issuecomment-1964444885
> mh, thinking a little bit more about this, any combination would probably be sub optimal for the camel-k POV, so I'm inclined to keep the implementation on the camel-k side and avoid to clutter camel-main
>
> @davsclaus @orpiske wdyt ?
Do you think it would be possible to make it work using the suggestion of abstracting it in a `MainOnCompletionPolicy` interface as I mentioned above? If yes, I think we could open a ticket and rework the code to allow you to implement the needed bits for Camel K without affecting the current behavior.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(camel-main): add options to let the shutdown strategy to wait or ignore inflight messages [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #13273:
URL: https://github.com/apache/camel/pull/13273#issuecomment-1964053912
In stream caching we have `anySpoolRule` (boolean)
So I wonder if we should use `ANY | ALL` as the enums.
durationCompletionRule = ANY | ALL
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(camel-main): add options to let the shutdown strategy to wait or ignore inflight messages [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #13273:
URL: https://github.com/apache/camel/pull/13273#issuecomment-1963602965
Yeah I can see the point that max does not have a inflight check, but that was also not its intention. But to say that after X message then shutdown. And during shutdown any inflight messages will be allowed to complete (within its timeout period).
And on the other handle the maxIdle, should only shutdown if no new messages for X period and that no inflight messages is processing (i.e. Camel is doing nothing).
So what you may ask for is to say max X and also being idle at the same time.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(camel-main): add options to let the shutdown strategy to wait or ignore inflight messages [camel]
Posted by "lburgazzoli (via GitHub)" <gi...@apache.org>.
lburgazzoli closed pull request #13273: feat(camel-main): add options to let the shutdown strategy to wait or ignore inflight messages
URL: https://github.com/apache/camel/pull/13273
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(camel-main): add options to let the shutdown strategy to wait or ignore inflight messages [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #13273:
URL: https://github.com/apache/camel/pull/13273#issuecomment-1963689695
> > Yeah I can see the point that max does not have a inflight check, but that was also not its intention. But to say that after X message then shutdown. And during shutdown any inflight messages will be allowed to complete (within its timeout period).
> > And on the other handle the maxIdle, should only shutdown if no new messages for X period and that no inflight messages is processing (i.e. Camel is doing nothing).
> > So what you may ask for is to say max X and also being idle at the same time.
>
> Do you think I should do something different ? like removing the options and combine max & idle so if they are turned on at the same time, then shutdown happens only when a number of messages have been processed and no activity in progress ?
Yes I think this is the correct and most useful use-cases
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(camel-main): add options to let the shutdown strategy to wait or ignore inflight messages [camel]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13273:
URL: https://github.com/apache/camel/pull/13273#issuecomment-1959626264
:star2: Thank you for your contribution to the Apache Camel project! :star2:
:robot: CI automation will test this PR automatically.
:camel: Apache Camel Committers, please review the following items:
* First-time contributors **require MANUAL approval** for the GitHub Actions to run
* You can use the command `/component-test (camel-)component-name1 (camel-)component-name2..` to request a test from the test bot.
* You can label PRs using `build-all`, `build-dependents`, `skip-tests` and `test-dependents` to fine-tune the checks executed by this PR.
* Build and test logs are available in the Summary page. **Only** [Apache Camel committers](https://camel.apache.org/community/team/#committers) have access to the summary.
* :warning: Be careful when sharing logs. Review their contents before sharing them publicly.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(camel-main): add options to let the shutdown strategy to wait or ignore inflight messages [camel]
Posted by "orpiske (via GitHub)" <gi...@apache.org>.
orpiske commented on code in PR #13273:
URL: https://github.com/apache/camel/pull/13273#discussion_r1499458392
##########
core/camel-main/src/main/java/org/apache/camel/main/MainDurationEventNotifier.java:
##########
@@ -110,10 +121,19 @@ protected void doNotify(CamelEvent event) {
if (complete) {
doneMessages.increment();
final int doneCount = doneMessages.intValue();
- final boolean result = doneCount >= maxMessages;
+ final int inflight = camelContext.getInflightRepository().size();
+
+ boolean result = doneCount >= maxMessages;
+ if (result && !maxMessagesIgnoreInflightExchanges) {
+ result = inflight == 0;
+ }
if (LOG.isTraceEnabled()) {
- LOG.trace("Duration max messages check {} >= {} -> {}", doneCount, maxMessages, result);
+ if (!maxMessagesIgnoreInflightExchanges) {
+ LOG.trace("Duration max messages check {}/{} >= {} -> {}", doneCount, inflight, maxMessages, result);
+ } else {
+ LOG.trace("Duration max messages check {} >= {} -> {}", doneCount, maxMessages, result);
+ }
Review Comment:
I'm wondering if we could have this in a way that retains the old logic. If you are processing a large batch of messages using the main, this part gets called quite a lot. So, this part was adjusted along with the performance fixes done around 4.0.0 development.
As it is now, it is forcing the code to hit a path that is costlier (i.e.; the inflight repository instance is volatile, then the call itself may or many not be inlined as the JIT may not be able to determine its type, etc).
So, I think having 2 separate methods would be better here.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(camel-main): add options to let the shutdown strategy to wait or ignore inflight messages [camel]
Posted by "lburgazzoli (via GitHub)" <gi...@apache.org>.
lburgazzoli commented on code in PR #13273:
URL: https://github.com/apache/camel/pull/13273#discussion_r1499977160
##########
core/camel-main/src/main/java/org/apache/camel/main/MainDurationEventNotifier.java:
##########
@@ -110,10 +121,19 @@ protected void doNotify(CamelEvent event) {
if (complete) {
doneMessages.increment();
final int doneCount = doneMessages.intValue();
- final boolean result = doneCount >= maxMessages;
+ final int inflight = camelContext.getInflightRepository().size();
+
+ boolean result = doneCount >= maxMessages;
+ if (result && !maxMessagesIgnoreInflightExchanges) {
+ result = inflight == 0;
+ }
if (LOG.isTraceEnabled()) {
- LOG.trace("Duration max messages check {} >= {} -> {}", doneCount, maxMessages, result);
+ if (!maxMessagesIgnoreInflightExchanges) {
+ LOG.trace("Duration max messages check {}/{} >= {} -> {}", doneCount, inflight, maxMessages, result);
+ } else {
+ LOG.trace("Duration max messages check {} >= {} -> {}", doneCount, maxMessages, result);
+ }
Review Comment:
would moving the inflight repository check inside the if enough ?
something like:
```java
int inflight = 0;
boolean result = doneCount >= maxMessages;
if (result && !maxMessagesIgnoreInflightExchanges) {
inflight = camelContext.getInflightRepository().size();
result = inflight == 0;
}
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(camel-main): add options to let the shutdown strategy to wait or ignore inflight messages [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #13273:
URL: https://github.com/apache/camel/pull/13273#issuecomment-1962317365
This can lead to waiting forever as if the system keeps taking in new inflight messages, as this will never issue the shutdown strategy to begin shutting down. The shutdown strategy automatic let inflight messages complete (it has a timeout of 45 seconds). So with this in mind I think the current way should cover this.
What is it that you see that dont work today?
Also if something needs to be added then why are the 2 options false and true by default, can we not have just one option instead. Camel already have too many options and can confuse users.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org