When a change to startup related code is made disable dev mode instrumentation

[geoand]

This is done because such code is meant to be run at startup so when it changes,
users expect to see it be rerun

Furthermore, add the -DdisableInstrumentation which allows a user to explicitly turn of instrumentation

[geoand]

I am not completely sure about the Arc change - it might catch to much stuff

[gsmet]

Hmmm, and what about entities are modified and the database schema needs an update? It this taken care of?

If not, it looks like a usability regression.

[geoand]

I have no idea if it's taken care of, but the new build item put in place by this PR, any build step can based on its own rules turn off instrumentation

[stuartwdouglas]

Yes, if entities are modified and the schema needs to be updated then instrumentation will not be used, as modifying method/field signatures or annotations means that instrumentation can't be used.

This is a bit of a corner case where you are actually coding logic that is run on startup, and only the logic changes.

[stuartwdouglas]

This should only apply if the class that was actually modified has a startup event, not if there is any startup event in the whole application.

[geoand]

This should only apply if the class that was actually modified has a startup event, not if there is any startup event in the whole application.

But doesn't it make sense that when there is a startup action it should always be executed?

I mean from a user's perspective, it will be hard to figure out and when the startup action is executed unless it always is.
That's the reason I did it the way I did as I believe consistency and being surprised as little as possible is important for users

[stuartwdouglas]

Instrumentation based reload only takes effect if you modify the bytecode only and no signatures. In general with our current restart unless you are modifying the actual start-up method itself re-executing the start-up method is a necessary but not desirable side effect of how we do restarts (i.e. it needs to run to get the app into a workable state). With instrumentation based reload the app is already in a working state, so re-running the start-up methods will only have an effect if the start-up methods themselves have been changed.

There are edge cases where you could have code on the call path from a startup method that you are calling, but if this is really a problem then IMHO we should just add an option to the mojo to disable instrumentation.

The reason why I added this option is that I have seen reports of apps taking multiple seconds to restart in dev mode (and startup methods can actually be a big culprit here). For a lot of the changes that developers make on a regular basis (i.e. logic errors in a method) this will really increase productivity.

[geoand]

I completely understand your reasoning and I do agree that ideally start-up should not be rerun.

However my main reasoning for doing it the way I did it is because:

1. I think its important to be consistent no matter what way the new user code takes effect
2. users are already familiar with the restart behavior - they actually do see it as a proper restart where everything is rerun

If you insist, I can certainly limit the scope of the change to what you mention

[stuartwdouglas]

Well in that case we should just remove it entirely. Its not worth maintaining something that will never be used.

[geoand]

You mean remove the instrumentation?

Why remove it just because of the startup thing?
Most applications probably won't have the startup capability anyway.

[stuartwdouglas]

I think most applications that are large enough to benefit from the instrumentation based approach will have startup code, especially because it is often startup code that slows the reload down. Even things like @PostConstruct on a JAX-RS resource could be considered startup code, as this will be re-run on a full startup but not on instrumentation based reload.

[geoand]

Hm... That's an interesting point indeed.

So I guess for now we should go with what you suggested and see what users think as they start using the instrumentation more

[mkouba]

FYI many extensions use @Observes StartupEvent as well, e.g. to initialize some beans at runtime.

[geoand]

PR completely overhauled - It's title and description have been updated to reflect what it now does

[stuartwdouglas]

We should probably make this easy to turn on/off via config, and also easy to quickly turn on/off in the dev console.

[stuartwdouglas]

I have changed this to use config to enable/disable instrumentation, and also expanded the test to cover it.

[geoand]

@stuartwdouglas seems like various tests are failing with java.lang.NoClassDefFoundError: io/quarkus/deployment/dev/DevConfig

[gsmet]

Wondering if this should backported? Instrumentation based reload is part of 1.11, right?

[geoand]

Yes it is: https://github.com/quarkusio/quarkus/blob/1.11/devtools/maven/src/main/java/io/quarkus/maven/DevMojo.java#L799

And yes, I believe this should be packported

Comments addressed