Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Registerable and discoverable Camel services #617

Closed
ppalaga opened this issue Jan 13, 2020 · 19 comments
Closed

Registerable and discoverable Camel services #617

ppalaga opened this issue Jan 13, 2020 · 19 comments

Comments

@ppalaga
Copy link
Contributor

ppalaga commented Jan 13, 2020

I stumbled upon this when I was rebasing my telegram branch on top of the current Camel master. It did not work because the endpoint and component configurers were not found. Those are currently looked up at runtime by inspecting the files in META-INF/services/org/apache/camel/configurer and we do not add them to the native image.

Clearly, the problem could be solved by adding META-INF/services/org/apache/camel/configurer & co. to the native image, but that kind of solution would not utilize the full potential of Quarkus build time augmentation.

I am currently having a PoC that provides a camel-quarkus specific implementation of FactoryFinderResolver. It resolves the services based on a single map that is filled at build time.

I wonder whether anybody can think of a better strategy here?

I'll share my code when I get all tests passing.

@lburgazzoli
Copy link
Contributor

I think all those services are already available from the registry

@ppalaga
Copy link
Contributor Author

ppalaga commented Jan 13, 2020

I think all those services are already available from the registry

I also suspected this might be true but I was not able to conceive any possibility how this fact could help. Maybe you mean that DefaultComponent.doInit() should prefer getting the configurers from the registry instead of from FactoryFinder?

@lburgazzoli
Copy link
Contributor

as far as I can remember the resolution always checks the registry and after the factory finder, this is how component, languages and other bits are actually resolved

if does not it seems an error but I remember it should be already the case for the configurer

@ppalaga
Copy link
Contributor Author

ppalaga commented Jan 13, 2020

@lburgazzoli
Copy link
Contributor

mh strange, I think that code should be fixed unless @davsclaus did it on purpose.

@ppalaga
Copy link
Contributor Author

ppalaga commented Jan 13, 2020

@davsclaus
Copy link
Contributor

the configurers are source code generated and loaded from classpath and as such these are not looked up anywhere. So its because you are doing something "different" in quarkus.

@ppalaga
Copy link
Contributor Author

ppalaga commented Jan 14, 2020

I have sent a PR to Camel apache/camel#3478 It makes DefaultComponent.doInit() to prefer getting the configurers from the registry before checking the FactoryFinder and it solves the original problem with missing configurers in the native mode.

Regardless of that, I think we should have a camel-quarkus specific implementation of FactoryFinderResolver that would be assembled at build time, so that the service property files do not need to get parsed at runtime. Let me share my code.

@lburgazzoli
Copy link
Contributor

lburgazzoli commented Jan 14, 2020

I'm not 100% sure about the need of a specific factory finder as we do already have the registry, we probably need to follow the same path when it is about tor resolve services so we don't need to have platform specific code.

As example the resolver for components, languages and dataformats do not need the factory finder at all so if we introduce a factory finder we need to use it everywhere otherwise it may result inconsistent.

@davsclaus
Copy link
Contributor

Isn't a problem with adding everything to the registry is that you clutter up and takes spaces there. Also for stuff that was only needed during build/startup time. Those will reside in Registry and wont be eligble to be removed.

For the endpoint and component configurers then yeah they may be used during runtime if you create new endpoints / components.

@lburgazzoli
Copy link
Contributor

lburgazzoli commented Jan 14, 2020

Yep but the factory finder does not actually solve the issue as the factory instance need to be in any case instantiated and put into another registry, otherwise we still need to look it up, do class for name, new instance at runtime.

Another option would be to actually discover such configurers at build time and register them through a recorder but the object won't be deleted as the component hold a reference so there's no real difference (IMHO) and this holds true for any object that is bound to a component.

@ppalaga
Copy link
Contributor Author

ppalaga commented Jan 14, 2020

In general, I do not mind prefering the Camel registry. It is there and can serve the purpose of instantiating services well.

The problem I am trying to solve here is, that we (camel-quarkus) still have code assuming that the FactoryFinders work well (incl. native), which they currently don't. I see several options how to fix that:

A. Brute force: always add all service property files to the native image and keep using DefaultFactoryFinderResolver / DefaultFactoryFinder which read the files at runtime.

B. Totally avoid using FactoryFinders. Provide an exception throwing FactoryFinder to make sure that our extensions do not use it at all. I am not sure this is possible and I am not sure this is a good idea, because third party extensions may still assume the FactoryFinders work.

C. My original proposal: Custom FactoryFinder assembled at build time, no need to include the property files in the native image.

Both A. and C. sound like acceptable solutions to me, C. being more in accordance with the spirit of Quarkus.

WDYT?

@lburgazzoli
Copy link
Contributor

In general, I do not mind prefering the Camel registry. It is there and can serve the purpose of instantiating services well.

The problem I am trying to solve here is, that we (camel-quarkus) still have code assuming that the FactoryFinders work well (incl. native), which they currently don't. I see several options how to fix that:

A. Brute force: always add all service property files to the native image and keep using DefaultFactoryFinderResolver / DefaultFactoryFinder which read the files at runtime.

B. Totally avoid using FactoryFinders. Provide an exception throwing FactoryFinder to make sure that our extensions do not use it at all. I am not sure this is possible and I am not sure this is a good idea, because third party extensions may still assume the FactoryFinders work.

C. My original proposal: Custom FactoryFinder assembled at build time, no need to include the property files in the native image.

The problem I see here is: what services do you add to the new factory finder "map" and what do you add to the registry ?

I guess at the end we do end up having the same service registered to different "registries", because we do not know what is the patter 3th party libraries are using, isn't it ?

May be we should have the factory finder to hooks into the registry or move every discovered service from the camel registry to the factory finder (but this requires some other changes in the FastCamelContext)

Both A. and C. sound like acceptable solutions to me, C. being more in accordance with the spirit of Quarkus.

An intermediate solution would be to have an option to

  1. store the service files in the native image
  2. have the factory finder to emit a warning when it loads from a file

WDYT?

@ppalaga
Copy link
Contributor Author

ppalaga commented Jan 14, 2020

C. My original proposal: Custom FactoryFinder assembled at build time, no need to include the property files in the native image.

The problem I see here is: what services do you add to the new factory finder "map"

The map is from property file paths (e.g. META-INF/services/org/apache/camel/configurer/telegram-component) to classes (e.g. org.apache.camel.component.telegram.TelegramComponentConfigurer.class) and my current implementation adds all available paths and classes to the map. Here is the code: #618

and what do you add to the registry ?

No change against what we currently do.

My understanding is that FactoryFinders and Registry serve different, but slightly overlapping purposes. Most notably, a FactoryFinder can find a class by path + name, which Registry cannot.

I guess at the end we do end up having the same service registered to different "registries", because we do not know what is the patter 3th party libraries are using, isn't it ?

Yes, FactoryFinders and the registry will contain slightly overlapping data, but not 100% identical.
The access keys are also different: FactoryFinders use hierarchic names (they include the resource path), while the registry names are flat.

May be we should have the factory finder to hooks into the registry or move every discovered service from the camel registry to the factory finder (but this requires some other changes in the FastCamelContext)

I am not sure FactoryFinders backed by a Registry covering all cases would be possible. Sounds like you assume the property file name is always equal to the name of the service in the registry. Based on @davsclaus comments in apache/camel#3478 (comment) I am not sure this is always true.

Besides, is your FactoryFinders backed by a Registry meant to happen in Camel or here?

Both A. and C. sound like acceptable solutions to me, C. being more in accordance with the spirit of Quarkus.

An intermediate solution would be to have an option to

  1. store the service files in the native image
  2. have the factory finder to emit a warning when it loads from a file

@lburgazzoli
Copy link
Contributor

lburgazzoli commented Jan 14, 2020

@ppalaga

After brainstorming a little bit more with @davsclaus we probably need to revisit how we load such services and we probably need a smart FactoryFinders which takes over the role of the registry for most of the discovery so we can keep the usual camel behaviour where registry is used to provide alternative implementations for what the it is defined by service factories.

We probably can have some build items that control how the discovered services are handled i.e.:

  1. a build item can control that some services need to be instantiated and they should probably a good candidates to be stored into the registry (i.e. components)
  2. a build item can control what services need to be discovered
  3. a build item can control what service files have to be backed into the final native image
  4. a build item can control what service files have to be "pre-loaded" in the form of a class

@ppalaga
Copy link
Contributor Author

ppalaga commented Jan 15, 2020

After brainstorming a little bit more with @davsclaus we probably need to revisit how we load such services and we probably need a smart FactoryFinders which takes over the role of the registry for most of the discovery so we can keep the usual camel behaviour where registry is used to provide alternative implementations for what the it is defined by service factories.

Sounds good.

We probably can have some build items that control how the discovered services are handled i.e.:

  1. a build item can control that some services need to be instantiated and they should probably a good candidates to be stored into the registry (i.e. components)

Yes, that's what we do for all (except filter-blacklisted) services ATM.

  1. a build item can control what services need to be discovered

Could you please define "need to be discovered"? Is it somehow to imply, that we are going to make some services non-discoverable at runtime (e.g. by not including the service file in the app)?

  1. a build item can control what service files have to be backed into the final native image
  2. a build item can control what service files have to be "pre-loaded" in the form of a class

What possible reasons are there for having both 3. and 4.? They seem to be serving the same purpose, don't they? Having just one of them would make our code simpler.

@ppalaga
Copy link
Contributor Author

ppalaga commented Jan 16, 2020

  1. a build item can control what service files have to be backed into the final native image
  2. a build item can control what service files have to be "pre-loaded" in the form of a class

If we had just 4. that would actually mean that the FactoryFinders work the same way for both JVM and native, so it would be easier to spot issues.

@lburgazzoli
Copy link
Contributor

will open a new issue with a proposal so we can have a better discussion

@ppalaga
Copy link
Contributor Author

ppalaga commented Jan 16, 2020

I have updated #618 Now it contains a minimalist implementation of 1., 2. and 4.

@ppalaga ppalaga changed the title Build time Camel service resolution Registerable and discoverable Camel services Jan 17, 2020
ppalaga added a commit to ppalaga/camel-quarkus that referenced this issue Jan 17, 2020
ppalaga added a commit to ppalaga/camel-quarkus that referenced this issue Jan 17, 2020
ppalaga added a commit to ppalaga/camel-quarkus that referenced this issue Jan 17, 2020
ppalaga added a commit to ppalaga/camel-quarkus that referenced this issue Jan 18, 2020
ppalaga added a commit to ppalaga/camel-quarkus that referenced this issue Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants