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

#36: make configuration value url optional #82

Merged
merged 1 commit into from
Oct 21, 2022
Merged

#36: make configuration value url optional #82

merged 1 commit into from
Oct 21, 2022

Conversation

turing85
Copy link
Contributor

Resolves #36

@middagj
Copy link
Contributor

middagj commented Oct 15, 2022

Pretty big, will review it later next week

@turing85
Copy link
Contributor Author

turing85 commented Oct 15, 2022

It looks actually bigger than it is. Most changes (about 100 file changes, give or take) are due to additional test modules, but the tests are mostly the same, with only minor changes.

I also took the liberty and did some general cleanup, e.g. replacing field- with constructor-injection where possible, some renamings for clarity, ...

Copy link
Contributor

@middagj middagj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the source code and put a few comments and skimmed the tests. If there is anything of particular interest in the tests, let me know.

@ppalaga Can you check if this would solve your issue?

@turing85
Copy link
Contributor Author

I checked the source code and put a few comments and skimmed the tests. If there is anything of particular interest in the tests, let me know.

Only important question is: is the amount of tests (and especially the amount of native image builds) okay? Right now, we are building six native images, a build runs for ~30 minutes.

@ppalaga
Copy link
Contributor

ppalaga commented Oct 20, 2022

@zhfeng could you please check if this solves apache/camel-quarkus#2857 for us?

@middagj
Copy link
Contributor

middagj commented Oct 20, 2022

Only important question is: is the amount of tests (and especially the amount of native image builds) okay? Right now, we are building six native images, a build runs for ~30 minutes.

That is really the downside of native builds. I don't know if there is an option to only do it once discussion is over.

@middagj
Copy link
Contributor

middagj commented Oct 20, 2022

Thanks again! I leave it open to get feedback from Camel people, but looks good to me.

@zhfeng
Copy link
Contributor

zhfeng commented Oct 21, 2022

Thanks @turing85 and I think it is good to resolve apache/camel-quarkus#2857.
So if there is a empty config, no ConnectionFactory bean will be generated and healthcheck will return 'UP'. Is it right? Aslo I think we need to update https://github.com/quarkiverse/quarkus-artemis/blob/main/docs/modules/ROOT/pages/index.adoc to add a section to explian the multiple named connection factory configuration and some notes.

@turing85
Copy link
Contributor Author

Thanks @turing85 and I think it is good to resolve apache/camel-quarkus#2857. So if there is a empty config, no ConnectionFactory bean will be generated and healthcheck will return 'UP'. Is it right?

This is partially correct. Basically, if we load the extension without any configuration, no injectable bean will be created, and the health endpoint will return UP, but the corresponding health check will not be loaded (see here). If we still want the health check to be loaded, then this should be an easy fix. But the check will not pick up the beans produced outside the extension. It would be possible to pick them up, however, we would have no chance of giving them meaningful names.

@zhfeng
Copy link
Contributor

zhfeng commented Oct 21, 2022

OK, I get it. In camel-qurakus-jms, we have a use case to create a custom ConnectionFactory such like

@Named("custom")
ConnectionFatory createCustomConnectionFactory() {
...
}

and to use it in the camel routers. So if we can find a way to enable the health check in this scenario, it would be great. I'm not sure what is about the meaningful names?

@turing85
Copy link
Contributor Author

turing85 commented Oct 21, 2022

The health of those connection factories are represented as key-value pairs in the health check. The key is a display name, the value is it status (see e.g. here). In the example you gave, the name is "custom" (I'd btw recommend to not use @Named, but other annotations like @Identifier or a custom scope). It would be possible to get all instances of the ConnectionFactory-interface, but we are not (easily) able to detect the annotation that qualifies the name.

@zhfeng
Copy link
Contributor

zhfeng commented Oct 21, 2022

Thanks for explainations. Can we create a follow up issue to track this improvement? and all of others are good to me. Thanks for your contribution again!

@turing85
Copy link
Contributor Author

turing85 commented Oct 21, 2022

@zhfeng I have created #84 to track the follow-up.

On a sidenote: I took another look at the source code of the pooled-jms extension, in particular PooledJmsWrapper, line 44. Am I correct that, for XA-enabled connection factories, connection pooling is not enabled? Scratch that. It was a brain fart.

@zhfeng
Copy link
Contributor

zhfeng commented Oct 21, 2022

No, I think the pooling is still enabled and just add support for transaction manager integration.

@turing85
Copy link
Contributor Author

Yeah, I saw the call to pooledJmsRuntimeConfigureConnectionFactory(...) after opening the code in an IDE :) As I said: Brain fart

@middagj middagj merged commit 0ff8034 into quarkiverse:main Oct 21, 2022
@turing85 turing85 deleted the feature/36-make-url-optional branch October 21, 2022 18:04
@turing85 turing85 added the enhancement New feature or request label Nov 3, 2022
@turing85 turing85 self-assigned this Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make quarkus.artemis.url optional
4 participants