Graceful shutdown strategy used as default one

[JiriOndrusek]

fixes #3179

Adds following behavior:

• in development mode, if no graceful shutdown timeout is set, there is no change (noShutdownStrategy is used)
• in other cases gracefulShutdownStrategy (same a is used in the Camel) is used
  @ppalaga I think that this should be mentioned in some doc, do you have a suggestion where to write it?

[aldettinger]

@JiriOndrusek Looks good, still few questions. Logically, it should make quarkus.camel.main.shutdown.timeout=xxx work. I don't see how to automatic test this but wonder if you have been able to check this manually on your machine ?

[jamesnetherton]

I don't see how to automatic test this

As a minimum, we could probably use QuarkusDevModeTest and verify that the correct shutdown strategy is configured on the CamelContext, depending on whether camel.main.shutdownTimeout is set.

[JiriOndrusek]

I'll try to add a test.
(@aldettinger I tested it locally with an app)
What do you think about a doc change regarding this?

[JiriOndrusek]

@jamesnetherton , @aldettinger I added 3 tests:

1. in core (not dev mode) - verifying that DefaultShutdownStrategy is used.
2. in main-devmode - CamelContextDefaultShutdownStrategyDevModeTest - if camel.main.shutdownTimeout is set, DefaulShutdownStrategy is used
3. in main-devmode - CamelContextNoShutdownStrategyDevModeTest - if camel.main.shutdownTimeout is not set, NoShutdownStrategy is used

[aldettinger]

@JiriOndrusek It looks better 👍

Do we have a test for core in DEV mode ? I mean without using camel-main in DEV mode. My current reading is that we will use the default shutdown strategy whereas no shutdown strategy would be preferrable ?

About the doc change: Are you talking about stating in the doc that the shutdown strategy has changed ? IMHO, I don't think it is needed.

[JiriOndrusek]

@aldettinger I would say that the use of DefaultShudownStrategy is preferable in all cases, with one exception - dev mode - because of quick redeploy. (but if in dev-mode camel.main.shutdownTimeout is set, default shutdown strategy will be used)

As for documentation. Current status was, that NoShutdownStrategy was used in all cases. This fix brings a change, which could affect some users. This is the reason, why I would say that doc should be updated - somewhere. (that default shutdown strategy was changed - mainly for not dev-mode)

[aldettinger]

I would say that the use of DefaultShudownStrategy is preferable in all cases, with one exception - dev mode - because of quick redeploy. (but if in dev-mode camel.main.shutdownTimeout is set, default shutdown strategy will be used)

I agree. Maybe I missread that with the current implementation, DefaultShudownStrategy is used in DEV mode when camel main is disabled. Do you agree with that ?

As for documentation. Current status was, that NoShutdownStrategy was used in all cases. This fix brings a change, which could affect some users. This is the reason, why I would say that doc should be updated - somewhere. (that default shutdown strategy was changed - mainly for not dev-mode)

Ok, it's clearer now. I think James has an idea that this could be in the migration guide. I think it is a good option.

[JiriOndrusek]

@aldettinger

DefaultShudownStrategy is used in DEV mode when camel main is disabled. Do you agree with that ?

Not entirely correct. DefaultShutdownStrategy is used in dev mode only if camel.main.shutdownTimeout is not null. (without taking into account whether camel main is disabled) - may not be the solution which we want. So in case that different condition should be applied, I'll change it.

[JiriOndrusek]

I hope that following statement will make the current behavior clear.

Current implementation is using only NoShutdownStrategy.
This fix changes strategy to DefaultShutdownStrategy,
Only if camel.main.shutdownTimeout is not set and dev mode is enabled - NoShutdownStrategy remains.

[JiriOndrusek]

I'll add change into the migration doc - https://github.com/apache/camel-quarkus/tree/main/docs/modules/ROOT/pages/migration-guide

[aldettinger]

@JiriOndrusek I'm +1 on adding the documentation to the migration guide

And indeed, I see it that now it does not take into account whether camel main is disabled. So the test coverage is good 👍

[JiriOndrusek]

@aldettinger documentation is added.

[aldettinger]

The migration guide is definitely the right place. We could merge this PR from my point of view 👍