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

Use ConnectionFactoryWrapperBuildItem in Quarkus #148

Closed
zhfeng opened this issue Feb 10, 2023 · 16 comments · Fixed by #149
Closed

Use ConnectionFactoryWrapperBuildItem in Quarkus #148

zhfeng opened this issue Feb 10, 2023 · 16 comments · Fixed by #149
Assignees

Comments

@zhfeng
Copy link
Contributor

zhfeng commented Feb 10, 2023

We need to use ConnectionFactoryWrapperBuildItem instead of ArtemisJmsWrapperBuildItem since quarkusio/quarkus#29753 get merged.

And I also create

@turing85
Copy link
Contributor

@zhfeng are you already implementing this issue here?

@zhfeng
Copy link
Contributor Author

zhfeng commented Feb 10, 2023

@turing85 yeah, I'm working on it and please assign to me.

@turing85
Copy link
Contributor

@zhfeng eager to see your implementation. I just tried it myself, but for quarkus 2.16.2.Final, I have - for some reason, define the version of quarkus-jms-spi-deployment explicitly, which bothers me somewhat.

@zhfeng
Copy link
Contributor Author

zhfeng commented Feb 10, 2023

@turing85 Hmm, quarkus-jms-spi-deployment is not in the io.quarkus:quarkus-bom?

@turing85
Copy link
Contributor

@zhfeng Nope. Looks like it should be added here, but it is not present in this file.

@zhfeng
Copy link
Contributor Author

zhfeng commented Feb 10, 2023

OK, I see and I can understand it is not there because quarkus-jms-spi-deployment can only be used by extensions such as quarkus-artemis and quarkus-pooled-jms. The application does not need it. Then we have to define version.

@zhfeng
Copy link
Contributor Author

zhfeng commented Feb 10, 2023

@turing85 is there any other issue when you tried?

@turing85
Copy link
Contributor

@zhfeng yes. The new build item is only accessible in the deployment module. I am not yet entirely sure how to translate all this. I think we want to keep the ArtemisJmsWrapper-class, and only transform this "at the last moment".

zhfeng added a commit to zhfeng/quarkus-artemis that referenced this issue Feb 10, 2023
@zhfeng
Copy link
Contributor Author

zhfeng commented Feb 10, 2023

@turing85 no problem and I just raise a PR for discussing. I can not catch your idea about "only transform this at the last moment" which mean only have it within quarkus 3 support?

@turing85
Copy link
Contributor

turing85 commented Feb 10, 2023

@zhfeng so my idea was to basically keep everything "as-is", and just add a BuildProducer<ConnectionFactoryWrapperBuildItem> connectionFactoryWrapperBuilder to the parameter-list ofArtemisJmsProcessor::configure and create a ConnectionFactoryWrapperBuildItem in the loop.

@zhfeng
Copy link
Contributor Author

zhfeng commented Feb 10, 2023

@turing85 I confused about your idea. It seems that we only need to consume ConnectionFactoryWrapperBuildItem but not produce it. On the other hand quarkus-pooled-jms produces it.

@turing85
Copy link
Contributor

turing85 commented Feb 10, 2023

@zhfeng OOOOOH! Okay. That was the reason I was unable to find a stage that produces the ArtemisJmsWrapperBuildItem 😄 That makes it much, much simpler.

@turing85
Copy link
Contributor

turing85 commented Feb 10, 2023

Sorry, @zhfeng, I missed that comment of yours.

OK, I see and I can understand it is not there because quarkus-jms-spi-deployment can only be used by extensions such as quarkus-artemis and quarkus-pooled-jms. The application does not need it. Then we have to define version.

I am not sure I agree. Are there other extensions that are not loaded by default? It is "just a bom", its purpose is to provide dependent module versions, it should be in there. We would never want a version of that extension that differs from the quarkus version.

@turing85
Copy link
Contributor

turing85 commented Feb 10, 2023

I opened PRs in quarkus to add it to the bom:
PR for main: quarkusio/quarkus#31074
PR for 2.16: quarkusio/quarkus#31075

@zhfeng
Copy link
Contributor Author

zhfeng commented Feb 10, 2023

Thanks for raising these PRs!

@turing85 turing85 linked a pull request Feb 10, 2023 that will close this issue
@zhfeng
Copy link
Contributor Author

zhfeng commented Feb 10, 2023

@turing85 It looks like we only need to add io.quarkus:quarkus-jms-spi-deployment in bom.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants