Fix #617 Registerable and discoverable Camel services #618

ppalaga · 2020-01-14T10:44:57Z

No description provided.

lburgazzoli

It would be nice to have some build time properties to control factory files to be included/excluded like:

quarkus.camel.factories.discovery.include = **/*
quarkus.camel.factories.discovery.exclude = org/apache/camel/something/*,org/apache/camel/xyz/**/*

so if a user know that some services/features are not needed, we can filter them out and avoid creating classes/instances.

lburgazzoli · 2020-01-16T17:17:23Z

...s/core/deployment/src/main/java/org/apache/camel/quarkus/core/deployment/BuildProcessor.java

+                    archives,
+                    "META-INF/services/org/apache/camel/component",
+                    "META-INF/services/org/apache/camel/language",
+                    "META-INF/services/org/apache/camel/dataformat")


we need to fine tune language and data-format as we can store into the registry only singleton services (this requires also some changes to the FastCamelContext as it assumes languages and dataformats are stored in the registry)

Yes we do have this problem as today but this is a good change to fix it

we need to fine tune language and data-format

Not sure how should we tune?

as we can store into the registry only singleton services

You seem to imply language and data-format are not singleton services, but could you plz. define "singleton services"?

The "tuning" here is about to carefully select which language/dataformat can be bound to the registry.

As example camel creates components only once so every time you use a schema, then you'll end up using the same component instance.

For languages and dataformat that's not always true and this is because you can use the same dataformat multiple time in the same route with a different configuration.

lburgazzoli · 2020-01-16T17:33:23Z

...s/core/deployment/src/main/java/org/apache/camel/quarkus/core/deployment/BuildProcessor.java

+                    archives,
+                    "META-INF/services/org/apache/camel/configurer")
+                    .filter(entry -> entry.getValue().getProperty("class") != null)
+                    .map(entry -> CamelServiceBuildItem.registeredOnly(


As future evolution, may be better not to expose such instances to the user through the registry, maybe better to try to bind them to the components at build time

I wonder how can we cover the case you mentioned on the chat - manually bind a second instance of a component under a different name?

I mean that we should not even bind them to the registry but scan the registry for components and bind related configurers through a recorder during STATIC_INIT

the registry for components and bind related configurers through a recorder during STATIC_INIT

Yes, that would be easy to do. But when a user adds a second instance of a component under a different name at runtime, our configurer setting code would not catch it.

yep but as the second instance is probably configured at runtime, the component will fallback to the "factory finder" way which is reasonable

ppalaga · 2020-01-16T20:37:31Z

It would be nice to have some build time properties to control factory files to be included/excluded like:
quarkus.camel.factories.discovery.include = **/*
quarkus.camel.factories.discovery.exclude = org/apache/camel/something/*,org/apache/camel/xyz/**/*

I wonder how the set of services defined by the extensions should be composed with these user facing selectors?

A. (extension-includes + user-includes) - user-excludes

or

B. extension-includes + (user-includes - user-excludes)

A. seems to make more sense. WDYT?

lburgazzoli · 2020-01-16T22:43:46Z

A. (extension-includes + user-includes) - user-excludes

Yes something like that should work

ppalaga · 2020-01-17T15:24:55Z

d82e823 implements the requested build time config.

The includes/excludes need to be defined incl. the META-INF/services/... prefix. I hope that's not an issue.

I vote for creating a new issue for the fine-tuning of the languages and data-formats.

I'd also create a new issue for replacing the current usage of CamelServiceFilterBuildItem with the new CamelServicePatternBuildItem because those ServiceFilters are just excludes.

ppalaga · 2020-01-17T15:34:56Z

4720897 rebased

lburgazzoli · 2020-01-17T16:03:22Z

.../core/deployment/src/main/java/org/apache/camel/quarkus/core/deployment/util/PathFilter.java

+
+        public PathFilter build() {
+            final List<String> incl = includePatterns;
+            includePatterns = null; // avoid leaking the collection trough reuse of the builder


not a huge issue in our case but my understanding is that invoking build() multiple time on the same builder should produce the same result

invoking build() multiple time on the same builder should produce the same result

If the collection is not set to null here the following sequence would be possible:

PathFilter.Builder b = new PathFilter.Builder(); b.include("foo/bar"); PathFilter pf1 = b.build(); b.include("bar/baz"); PathFilter pf2 = b.build();

where pf1 and pf2 would have set the same list instance in their includePatterns fields. That would be really unwanted. That's a true reference leak that harms the immutability of PathFilter.

I hold re-using builders for rather unusual and so I find breaking the re-use better than copying the list in the PathFilter constructor as a way to ensure immutability.

At least document it as I think no-one expect the builder to be re-set once you invoke build()

Done at https://github.com/apache/camel-quarkus/pull/618/files#diff-7d0dfbc8ca22f0eb14e77322fcda0902R143

Well another, in my opinion, cleaner option is to copy the collection in each object built making it immutable. A variation of this is used in most of the framework that generates builders, like Lombok and similars one.

My 2c.

lburgazzoli · 2020-01-17T16:04:49Z

I vote for creating a new issue for the fine-tuning of the languages and data-formats.

if you have time, it would be nice to at least remove them from the registry and use the default camel discovery for languages and dataformats, we can fine tune them later one but at least we are on the safe side

ppalaga · 2020-01-17T16:56:19Z

remove them from the registry and use the default camel discovery for languages and dataformats

Done in 73f59ef

lburgazzoli · 2020-01-17T20:58:35Z

ok to test

lburgazzoli · 2020-01-18T09:56:11Z

conflicts

ppalaga · 2020-01-18T11:04:13Z

7e11340 rebased

ppalaga · 2020-01-20T08:00:43Z

ok to test

oscerd · 2020-01-20T09:13:55Z

ok to test

lburgazzoli · 2020-01-20T10:12:36Z

The build seems to be running

https://builds.apache.org/view/C/view/Apache%20Camel/job/camel-quarkus-pr/538/

even if it does not show up here

ppalaga · 2020-01-20T10:29:39Z

This should be fixed by the Do not register languages and dataformats ... commit of #618

This reverts commit e6920d0. We decided to adopt another approach

… parallel with conflicting configs

ppalaga · 2020-01-20T16:26:59Z

869af41 rebased and fixed a subtle bug that prevented the Dozer tests from passing

ppalaga · 2020-01-20T16:27:14Z

ok to test

lburgazzoli · 2020-01-20T17:18:26Z

ok to test

ppalaga mentioned this pull request Jan 14, 2020

Registerable and discoverable Camel services #617

Closed

ppalaga force-pushed the 200113-fast-factory-finder branch from 9079f97 to 3843984 Compare January 16, 2020 17:07

lburgazzoli requested changes Jan 16, 2020

View reviewed changes

lburgazzoli reviewed Jan 16, 2020

View reviewed changes

ppalaga changed the title ~~Fix #Build time FactoryFinders~~ Fix #617 Registerable and discoverable Camel services Jan 17, 2020

ppalaga force-pushed the 200113-fast-factory-finder branch from 3843984 to d82e823 Compare January 17, 2020 15:15

ppalaga marked this pull request as ready for review January 17, 2020 15:25

ppalaga force-pushed the 200113-fast-factory-finder branch from d82e823 to 4720897 Compare January 17, 2020 15:34

lburgazzoli reviewed Jan 17, 2020

View reviewed changes

ppalaga force-pushed the 200113-fast-factory-finder branch from 73f59ef to 671803a Compare January 17, 2020 17:11

ppalaga force-pushed the 200113-fast-factory-finder branch from 671803a to 7e11340 Compare January 18, 2020 11:03

ppalaga added 4 commits January 20, 2020 15:56

Revert "Fix apache#620 Introduce CamelServiceInfo transformers"

2597d80

This reverts commit e6920d0. We decided to adopt another approach

Avoid using field injection in the core NativeImageProcessor

c631528

Fix apache#617 Registerable and discoverable Camel services

1645a33

Do not register languages and dataformats so that they can be used in…

869af41

… parallel with conflicting configs

ppalaga force-pushed the 200113-fast-factory-finder branch from 7e11340 to 869af41 Compare January 20, 2020 16:25

lburgazzoli approved these changes Jan 20, 2020

View reviewed changes

lburgazzoli merged commit a2e1dc2 into apache:master Jan 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #617 Registerable and discoverable Camel services #618

Fix #617 Registerable and discoverable Camel services #618

ppalaga commented Jan 14, 2020

lburgazzoli left a comment •

edited

Loading

lburgazzoli Jan 16, 2020

lburgazzoli Jan 16, 2020

ppalaga Jan 16, 2020

lburgazzoli Jan 16, 2020

lburgazzoli Jan 16, 2020

ppalaga Jan 16, 2020

lburgazzoli Jan 16, 2020

ppalaga Jan 17, 2020

lburgazzoli Jan 17, 2020

ppalaga commented Jan 16, 2020

lburgazzoli commented Jan 16, 2020

ppalaga commented Jan 17, 2020

ppalaga commented Jan 17, 2020

lburgazzoli Jan 17, 2020

ppalaga Jan 17, 2020 •

edited

Loading

lburgazzoli Jan 17, 2020

ppalaga Jan 17, 2020

valdar Jan 17, 2020

lburgazzoli commented Jan 17, 2020 •

edited

Loading

ppalaga commented Jan 17, 2020

lburgazzoli commented Jan 17, 2020

lburgazzoli commented Jan 18, 2020

ppalaga commented Jan 18, 2020

ppalaga commented Jan 20, 2020

oscerd commented Jan 20, 2020

lburgazzoli commented Jan 20, 2020

ppalaga commented Jan 20, 2020

ppalaga commented Jan 20, 2020

ppalaga commented Jan 20, 2020

lburgazzoli commented Jan 20, 2020

Fix #617 Registerable and discoverable Camel services #618

Fix #617 Registerable and discoverable Camel services #618

Conversation

ppalaga commented Jan 14, 2020

lburgazzoli left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppalaga commented Jan 16, 2020

lburgazzoli commented Jan 16, 2020

ppalaga commented Jan 17, 2020

ppalaga commented Jan 17, 2020

Choose a reason for hiding this comment

ppalaga Jan 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lburgazzoli commented Jan 17, 2020 • edited Loading

ppalaga commented Jan 17, 2020

lburgazzoli commented Jan 17, 2020

lburgazzoli commented Jan 18, 2020

ppalaga commented Jan 18, 2020

ppalaga commented Jan 20, 2020

oscerd commented Jan 20, 2020

lburgazzoli commented Jan 20, 2020

ppalaga commented Jan 20, 2020

ppalaga commented Jan 20, 2020

ppalaga commented Jan 20, 2020

lburgazzoli commented Jan 20, 2020

lburgazzoli left a comment •

edited

Loading

ppalaga Jan 17, 2020 •

edited

Loading

lburgazzoli commented Jan 17, 2020 •

edited

Loading