Publish discovered RoutesBuilders via CamelBeanBuildItem #358
Conversation
|
This should have been a draft PR. Sorry. |
|
I'm wonder if it worth doing that as there's basic dependency injection in camel that works out of the box (example) and if people need to use MicroProfile / CDI injection then they could simply annotate the route with ApplicationScope or any similar annotation (we still need to fix the problem of having the same route discovered at runtime and at build time, but that's something we need to do in any case). I would really like to keep instantiating routes in static init as much as possible. |
Maybe I am missing something trivial, but adding
Good point, let me add a test for that. |
I think it does not work because camel discovers it on the classpath and load it on ts own so it bypass ArC.
|
|
@lburgazzoli I finally figured out why you expect the injection & co in an Actually the current proposal (mostly) eliminates the duplicate route initialization problem we discussed in the chat. There are still two route builders seen by CamelMain, but now they are the same instance (bc. both come from Arc and Arc correctly returns the same one anytime it is queried). The same instance won't get initialized and added to the Context twice thanks to Let me think how to proceed here. |
|
I think we really need to get rid of the double discovery as having two identical builder is confusing and inefficient and it is a generic issue that has broader impacts than camel-quarkus |
|
b14487b is the most elegant fix I was able to find. All RoutesBuilders are instantiated by Arc and queried via the Registry. CamelRoutesBuilderBuildItem removed altogether. Not a WiP anymore. |
|
does this means that route builder classes are now instantiated at runtime only ? |
Yes, the constructors are called from main() not from a clinit. |
| @@ -232,7 +228,6 @@ CamelMainBuildItem main( | |||
| CamelContextBuildItem context, | |||
| CamelRoutesCollectorBuildItem routesCollector, | |||
| List<CamelMainListenerBuildItem> listeners, | |||
| List<CamelRoutesBuilderBuildItem> routesBuilders, | |||
There was a problem hiding this comment.
I should be able to add routes that are instantiated at build time, i.e. if I use camel annotation for DI I do not need ArC at all thus I can optimize a bit my application.
There was a problem hiding this comment.
That would re-introduce the double initialization of the routes.
I was thinking of filtering by @ApplicationScoped that can be done well using Jandex: Builders having @ApplicationScoped would behave like in the current proposal and all others could go via CamelRoutesBuilderBuildItem. This would be more complex and thus potentially more buggy and harder to maintain. Maybe you have a better idea?
There was a problem hiding this comment.
Camel has the ability to do class-path scanning to discover routes which can be activated by configuring which packages camel has to scan for RoutesBuilder classes (already supported by other platform such as Spring, CDI, Main) so we can mimic that behaviour at build time with a quarkus.camel.* option (note that this can activated also right now through camel main properties but class path scanning would happen at runtime).
By doing that, routes builders can be added to the context by:
- producing a
CamelBeanBuildItem - using a
MainListener - using an injected
CamelContext - using a CDI annotation such as
@ApplicationScoped - listing the packages to scan for auto discovery (as far as I remember discovered routes have lower precedence than those available from the container thus discarded if the same route class is also provided by the container)
An additional option that has been discussed would be to leverage camel's annotation @BindToRegistry which would act like a cross platform lite DI.
There was a problem hiding this comment.
Yes, I agree. You perhaps meant CamelRoutesBuilderBuildItem rather than CamelBeanBuildItem?
The ultimate aim of this PR is to make the @Inject, @ConfigProperty, etc. work in RoutesBuilders (see the tests in this PR that currently fail). I agree that the problem of double Route initialization is actually the root cause.
My previous proposal to solve both problems by delegating the creation of all RouteBuilders to Arc was rejected for boot performance reasons which I found quite legitimate.
I proposed an alternative approach where Arc beans are thrown away from the list provided via CamelRoutesBuilderBuildItem, but I see no feedback for that.
In between, I looked how to implement that reliably, and found that the list of Arc beans could perhaps be pulled from Arc itself like this: ppalaga@cdbfd19#diff-ec6205752018e84a80fcd9eaa92af189R203-R209 (once they change the visibility of some methods, I asked the Arc guys about that). What do you think about the linked solution?
There was a problem hiding this comment.
No CamelBeanBuildItem is - IMHO - the right bean to use as every route bound to the registry is then automatically discovered so CamelRoutesBuilderBuildItem is superfluous but that is a minor issue.
The problem about using ArC in STATIC_INIT is that it may not be aware of the "runtime beans" at that point in time, at least in my experience.
You may think to override the route discovery class and apply the filter there.
|
This is IMHO bad as we are de optimizing the app |
ppalaga
force-pushed
the
i136
branch
2 times, most recently
from
1624cbd
to
ac51838
Compare
|
About ac51838:
All open questions are solved from my PoV |
|
Hm... looking at the test failures, it looks like there is one more problem: The registry sees only CDI beans instantiated eagerly - e.g. if they have |
|
I solved the mentioned problem by marking all RouteBuilder classes as unremovable via UnremovableBeanBuildItem. |
|
|
||
| /** | ||
| * Holds the {@link RoutesBuilder} {@link RuntimeValue}. | ||
| * A {@link MultiBuildItem} holding class names of all {@link RoutesBuilder} implementations found via Jandex. |
There was a problem hiding this comment.
Not very sure if implementations found via Jandex really make sense as any extension developer can create it programmatically.
There was a problem hiding this comment.
True, let me reword it.
| // | ||
| // Register routes as reflection aware as camel-main main use reflection | ||
| // to bind beans to the registry | ||
| // | ||
| CamelSupport.getRouteBuilderClasses(view).forEach(name -> { | ||
| reflectiveClass.produce(new ReflectiveClassBuildItem(true, false, name)); | ||
| camelRoutesBuilders.stream().forEach(dotName -> { |
There was a problem hiding this comment.
stream() is not needed
| camelRoutesBuilders.stream() | ||
| .map(CamelRoutesBuilderClassBuildItem::getDotName) | ||
| .filter(dotName -> !arcBeanClasses.contains(dotName)) | ||
| .forEach(dotName -> { |
There was a problem hiding this comment.
maybe map + collect would be more idiomatic
There was a problem hiding this comment.
Yeah, that would be possible now. The previous revisions did not allow that. Let me refactor.
...s/core/deployment/src/main/java/org/apache/camel/quarkus/core/deployment/BuildProcessor.java
Outdated
Show resolved
Hide resolved
...s/core/deployment/src/main/java/org/apache/camel/quarkus/core/deployment/BuildProcessor.java
Show resolved
Hide resolved
...src/main/java/org/apache/camel/quarkus/core/deployment/CamelRoutesBuilderClassBuildItem.java
Outdated
Show resolved
Hide resolved
|
All requests by @lburgazzoli addressed in 22fed9b |
|
ok to test |
|
Refer to this link for build results (access rights to CI server needed): Build result: FAILURE[...truncated 1.83 MB...] at sun.reflect.GeneratedMethodAccessor255.invoke (Unknown Source) at sun.reflect.DelegatingMethodAccessorImpl.invoke (DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke (Method.java:498) at io.quarkus.deployment.ExtensionLoader$1.execute (ExtensionLoader.java:941) at io.quarkus.builder.BuildContext.run (BuildContext.java:415) at org.jboss.threads.ContextClassLoaderSavingRunnable.run (ContextClassLoaderSavingRunnable.java:35) at org.jboss.threads.EnhancedQueueExecutor.safeRun (EnhancedQueueExecutor.java:2011) at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.doRunTask (EnhancedQueueExecutor.java:1535) at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run (EnhancedQueueExecutor.java:1426) at java.lang.Thread.run (Thread.java:748) at org.jboss.threads.JBossThread.run (JBossThread.java:479)[ERROR] [ERROR] Re-run Maven using the -X switch to enable full debug logging.[ERROR] [ERROR] For more information about the errors and possible solutions, please read the following articles:[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException[ERROR] [ERROR] After correcting the problems, you can resume the build with the command[ERROR] mvn -rf :camel-quarkus-integration-test-corechannel stoppedAdding one-line test results to commit status...Setting status of 22fed9b to FAILURE with url https://builds.apache.org/job/camel-quarkus-pr/388/ and message: 'FAILURE 113 tests run, 3 skipped, 0 failed.'Using context: default |
|
…amelRoutesBuilderBuildItem
classes just once.
ppalaga
changed the title
|
About 53252e8: Fixed the first two commits (should pass in the native mode now) and removed the third one, which requires more work. Will send the third one separately once I make it work. |
|
ok to test |
|
Refer to this link for build results (access rights to CI server needed): |
The main idea here is to provide a custom
QuarkusInjectorthat first looks up the given type in the Arc container before falling back to direct instantiation. At the same time RoutesBuilders are not instantiated by us, we rather supply the RoutesBuilder class names to Camel main to instantiate them via ourQuarkusInjector.Let's discuss whether the above is a good idea.
I see the following possible risks:
Both 1. and 2. could get mitigated for RoutesBuilders that do not contain any injection points by filtering out those and instantiating them as we do now, whereas only RoutesBuilders with injection points would be instantiated as sketched in this proposal. The question is whether the more complex implementation is worth the effort.