feat(camel-main): add options to let the shutdown strategy to wait or ignore inflight messages

[lburgazzoli]

Description

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 filed for the change (usually before you start working on it).

Apache Camel coding standards and style

• [ ] I checked that each commit in the pull request has a meaningful subject line and body.

• [ ] I have run mvn clean install -DskipTests locally and I have committed all auto-generated changes

[github-actions[bot]]

: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 have access to the summary.

• :warning: Be careful when sharing logs. Review their contents before sharing them publicly.

[lburgazzoli]

@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

[davsclaus]

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.

[lburgazzoli]

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 some 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 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
• 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 before triggering the shutdown so to preserve the current behavior, I added a second option

[lburgazzoli]

If you don't see this generally applicable to camel-main, we can retain the behavior in camel-k without problems

[davsclaus]

Yeah lets give this some thinking on monday

[davsclaus]

Typo

Ingore -> Ignore

[davsclaus]

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.

[davsclaus]

When this is done, then you need to add these 2 new options in camel-spring-boot project

[lburgazzoli]

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 ?

[davsclaus]

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

[orpiske]

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. Then, 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.

Edit: additionally, it has a bonus of simplifying the MainDurationEventNotifier constructor, which is already way too cumbersome.

[lburgazzoli]

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 ?

[davsclaus]

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.

[lburgazzoli]

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 ?

[davsclaus]

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

[orpiske]

IMHO, I think an enum would be better ... something like QuiescePolicy -> { INCLUSIVE, EXCLUSIVE }

[lburgazzoli]

Something like this ?

DurationStrategies { AND, OR }

[davsclaus]

In stream caching we have anySpoolRule (boolean)

So I wonder if we should use ANY | ALL as the enums.

durationCompletionRule = ANY | ALL

[lburgazzoli]

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 ?

[davsclaus]

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

[orpiske]

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.

[lburgazzoli]

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