Introduce ConnectionFactoryWrapperBuildItem #29753

zhfeng · 2022-12-08T09:52:14Z

This is from the disussion https://groups.google.com/g/quarkus-dev/c/thdTCj-XNUU

turing85 · 2022-12-08T10:48:18Z

...loyment/src/main/java/io/quarkus/deployment/builditem/ConnectionFactoryWrapperBuildItem.java

+ * A build item that can be used to wrap ConnectionFactory
+ */
+public final class ConnectionFactoryWrapperBuildItem extends SimpleBuildItem {
+    private final Function<Object, Object> wrapper;


How does an intended use look like? Is it really necessary that the wrapper holds a Function<Object, Object>, or can we limit the bounds of this function somehow?

Moreover, I think that the Function instance needs to be produced by a recorder method, or?

Moreover, I think that the Function instance needs to be produced by a recorder method, or?

Not necessarily. This is why I asked for a sample usage. In quarkiverse/quarkus-artemis, we are doing something similar already, where we pass the wrapper down to the recorder.

I think we produce the wrapper in a record, just like https://github.com/quarkiverse/quarkus-pooled-jms/blob/main/deployment/src/main/java/io/quarkiverse/messaginghub/pooled/jms/deployment/PooledJmsProcessor.java#L35-L37

Well, if the Function is used at runtime then you can't simply provide an instance at build time but use a proxy produced by a recorder instead. In your case, the ArtemisJmsWrapper used in ArtemisJmsRecorder#extractConnectionFactory() is a proxy produced by ArtemisJmsRecorder#getDefaultWrapper() (or similar recorder method).

Yeah, it is excatly we are doing in quarkus-artemis and quarkus-pooled-jms.

...loyment/src/main/java/io/quarkus/deployment/builditem/ConnectionFactoryWrapperBuildItem.java

mkouba · 2022-12-08T13:16:52Z

...loyment/src/main/java/io/quarkus/deployment/builditem/ConnectionFactoryWrapperBuildItem.java

+import io.quarkus.builder.item.SimpleBuildItem;
+
+/**
+ * A build item that can be used to wrap ConnectionFactory


It's not clear what ConnectionFactory is being wrapped. It should be JMS-specific, or?

Yeah, it should be JMS-specific. I was thinking to use Function<ConnectionFactory, Object> but it needs to introduce a jakarta.jms-api dependency in quarkus-core, is it feasible?

-1

I wonder if we should introduce a JMS SPI module instead, e.g. something like quarkus-jms-spi. This module can be consumed by all JMS extensions.

You mean add a module jms-spi in core?

It depends. It could be a module in the quarkus/core or in quarkiverse/quarkus-artemis. @maxandersen WDYT?

Agree that it should at least be in extensions. I think a common quarkiverse repo makes the most sense but its so small that including it is not unreasonable. The one thing that gives me pause about this is just that people have been complaining about IDE setups being slow to index, so adding another major API jar might make things worse. In the long term we should move to some internal split layout to address that problem, but thats not happening in the short term.

Thanks all for reviewing. So it looks like we agree to add this BuildItem in a spi extension. Something similar with https://github.com/quarkusio/quarkus/tree/main/extensions/jaxrs-spi, is it OK?

@gsmet @n1hility can you take a review and is it OK for merging?

So it looks like we agree to add this BuildItem in a spi extension

Not sure what makes you think that. I think all of us are not overly excited to have this in core and would prefer a Quarkiverse extension. I will merge it anyway but don't be surprised if at some point we decide to move it out. Core is extremely crowded and the bar to add something in it is very high.

I will do a last review round.

OK, I see and thankd for reviewing.

zhfeng · 2023-01-30T08:54:44Z

Hi @mkouba ,

I just read the Jakarta CDI spec about InterceptionFactory and wonder if this is an option for wrapping ConnecitonFactory? I know this is not supported in Quarkus Arc right now but just curious there is any issue about implmentation from technical perspective?

mkouba · 2023-01-30T09:04:22Z

Hi @mkouba ,

I just read the Jakarta CDI spec about InterceptionFactory and wonder if this is an option for wrapping ConnecitonFactory? I know this is not supported in Quarkus Arc right now but just curious there is any issue about implmentation from technical perspective?

Yes, there are some technical issues with InterceptionFactory - see #3532 for more information.

zhfeng · 2023-01-30T10:08:30Z

Thanks @mkouba and it is also the reason for not support interception in method produce bean generation? see https://github.com/quarkusio/quarkus/blob/main/independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/BeanGenerator.java#L441

mkouba · 2023-01-30T16:02:29Z

Thanks @mkouba and it is also the reason for not support interception in method produce bean generation? see https://github.com/quarkusio/quarkus/blob/main/independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/BeanGenerator.java#L441

Well, the methods of a returned value from a CDI producer method are never intercepted because the CDI container does not control the lifecycle of the returned instance.

gsmet · 2023-02-02T09:31:14Z

...eployment/src/main/java/io/quarkus/jms/spi/deployment/ConnectionFactoryWrapperBuildItem.java

+
+import javax.jms.ConnectionFactory;
+
+import org.wildfly.common.Assert;


We definitely don't want to use a WildFly class here.

OK, I remove it.

extensions/pom.xml

gsmet · 2023-02-02T09:37:50Z

...loyment/src/main/java/io/quarkus/deployment/builditem/ConnectionFactoryWrapperBuildItem.java

+import io.quarkus.builder.item.SimpleBuildItem;
+
+/**
+ * A build item that can be used to wrap ConnectionFactory


So it looks like we agree to add this BuildItem in a spi extension

Not sure what makes you think that. I think all of us are not overly excited to have this in core and would prefer a Quarkiverse extension. I will merge it anyway but don't be surprised if at some point we decide to move it out. Core is extremely crowded and the bar to add something in it is very high.

I will do a last review round.

gsmet

I added some small comments here and there. Please have a look.

...eployment/src/main/java/io/quarkus/jms/spi/deployment/ConnectionFactoryWrapperBuildItem.java

gsmet · 2023-02-02T10:00:45Z

...eployment/src/main/java/io/quarkus/jms/spi/deployment/ConnectionFactoryWrapperBuildItem.java

+/**
+ * A build item that can be used to wrap the JMS ConnectionFactory
+ */
+public final class ConnectionFactoryWrapperBuildItem extends SimpleBuildItem {


BTW, you aware that you will be able to produce only one of these?

Not sure exactly for what it is used but that probably means you won't be able to mix several JMS extensions (not sure if it makes sense).

As far as I know, only quarkus-pooled-jms will produce this BuildItem to provide pooling and JTA integration capabilities currently . Other JMS extensions can consume it.

gsmet

I will merge it after the 3.0 switch in main as we need to upgrade the JMS API version and I won't do last minute change to the transformation script now.

quarkus-bot · 2023-02-06T18:35:07Z

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

gsmet · 2023-02-07T08:37:45Z

@zhfeng do you need this thing sooner and should we backport the first commit to the next 2.16 micro? (I will do it, just asking if it's useful)

zhfeng · 2023-02-07T13:42:27Z

Thanks @gsmet - yeah, please backport it to 2.16.x

This MR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [com.diffplug.spotless:spotless-maven-plugin](https://github.com/diffplug/spotless) | build | minor | `2.32.0` -> `2.33.0` | | [io.quarkus:quarkus-maven-plugin](https://github.com/quarkusio/quarkus) | build | patch | `2.16.1.Final` -> `2.16.2.Final` | | [io.quarkus:quarkus-universe-bom](https://github.com/quarkusio/quarkus-platform) | import | patch | `2.16.1.Final` -> `2.16.2.Final` | --- ### Release Notes <details> <summary>diffplug/spotless</summary> ### [`v2.33.0`](https://github.com/diffplug/spotless/blob/HEAD/CHANGES.md#2330---2023-01-26) ##### Added - `ProcessRunner` has added some convenience methods so it can be used for maven testing. ([#1496](diffplug/spotless#1496)) - `ProcessRunner` allows to limit captured output to a certain number of bytes. ([#1511](diffplug/spotless#1511)) - `ProcessRunner` is now capable of handling long-running tasks where waiting for exit is delegated to the caller. ([#1511](diffplug/spotless#1511)) - Allow to specify node executable for node-based formatters using `nodeExecutable` parameter ([#1500](diffplug/spotless#1500)) ##### Fixed - The default list of type annotations used by `formatAnnotations` has had 8 more annotations from the Checker Framework added [#1494](diffplug/spotless#1494) ##### Changes - **POTENTIALLY BREAKING** Bump minimum JRE from 8 to 11, next release likely to bump bytecode to Java 11 ([#1514](diffplug/spotless#1514) part 1 of [#1337](diffplug/spotless#1337)) - Rename `YamlJacksonStep` into `JacksonYamlStep` while normalizing Jackson usage ([#1492](diffplug/spotless#1492)) - Convert `gson` integration to use a compile-only source set ([#1510](diffplug/spotless#1510)). - \*\* POTENTIALLY BREAKING\*\* Removed support for KtLint 0.3x and 0.45.2 ([#1475](diffplug/spotless#1475)) - `KtLint` does not maintain a stable API - before this MR, we supported every breaking change in the API since 2019. - From now on, we will support no more than 2 breaking changes at a time. - NpmFormatterStepStateBase delays `npm install` call until the formatter is first used. This enables better integration with `gradle-node-plugin`. ([#1522](diffplug/spotless#1522)) - Bump default `ktlint` version to latest `0.48.1` -> `0.48.2` ([#1529](diffplug/spotless#1529)) - Bump default `scalafmt` version to latest `3.6.1` -> `3.7.1` ([#1529](diffplug/spotless#1529)) </details> <details> <summary>quarkusio/quarkus</summary> ### [`v2.16.2.Final`](https://github.com/quarkusio/quarkus/releases/tag/2.16.2.Final) [Compare Source](quarkusio/quarkus@2.16.1.Final...2.16.2.Final) ##### Complete changelog - [#30976](quarkusio/quarkus#30976) - Metrics - check if index contains class before attempting to use it - [#30965](quarkusio/quarkus#30965) - JandexBeanInfoAdapter.getMetricAnnotationsThroughStereotype is not null safe - [#30959](quarkusio/quarkus#30959) - Return text from /q/metrics when the Accept header contains html - [#30953](quarkusio/quarkus#30953) - Fix OIDC capability string - [#30947](quarkusio/quarkus#30947) - Ignore interface/class without default constructs fields in SB config - [#30940](quarkusio/quarkus#30940) - Use SchemaType.ARRAY instead of "ARRAY" for native support - [#30919](quarkusio/quarkus#30919) - Compilation to native fails, when quarkus-smallrye-openapi is included - [#30916](quarkusio/quarkus#30916) - Add AppCDS documentation - [#30896](quarkusio/quarkus#30896) - Quarkus spring-boot-properties extension unable to handle complex configuration. - [#30878](quarkusio/quarkus#30878) - Bump postgresql from 42.5.2 to 42.5.3 - [#30866](quarkusio/quarkus#30866) - Only run the quickstart compilation for main - [#30851](quarkusio/quarkus#30851) - Fixed return type typo in smallrye graphQL guide - [#30844](quarkusio/quarkus#30844) - Fixed greeting in getting started guide - [#30839](quarkusio/quarkus#30839) - Fix handling of Accept header in graphQL - [#30833](quarkusio/quarkus#30833) - Update docs to show BuildProducer use as method parameter instead of field - [#30828](quarkusio/quarkus#30828) - Make OIDC session cookie same site lax by default - [#30826](quarkusio/quarkus#30826) - Caffeine - Automatically register metrics cache impls if Micrometer is around - [#30825](quarkusio/quarkus#30825) - Fix comment about Caffeine optimization - [#30823](quarkusio/quarkus#30823) - Change accept header to valid plain text in micrometer documentation - [#30821](quarkusio/quarkus#30821) - Packaging type -Dquarkus.package.create-appcds=true isn't documented - [#30815](quarkusio/quarkus#30815) - Update SmallRye Config to 2.13.2 - [#30812](quarkusio/quarkus#30812) - Manage the apache-mime4j dependency - [#30806](quarkusio/quarkus#30806) - */* in Accept header is ignored if not listed as the first item - [#30805](quarkusio/quarkus#30805) - MailTemplateInstance with attachments - [#30803](quarkusio/quarkus#30803) - Support file and byte array attachments in `MailTemplateInstance` - [#30797](quarkusio/quarkus#30797) - OIDC login not work - [#30783](quarkusio/quarkus#30783) - <artifactId> uses 'quarkus.platform.artifact-id' property - [#30778](quarkusio/quarkus#30778) - Avoid creating 3 Liquibase MongoDB instances for startup operations - [#30776](quarkusio/quarkus#30776) - Ensure that AwsProxyRequestContext can be used with [@Context](https://github.com/Context) in RESTEasy Reactive - [#30767](quarkusio/quarkus#30767) - Remove duplicate notification of SseBroadcaster's onErrorListeners - [#30765](quarkusio/quarkus#30765) - Bump postgresql from 42.5.1 to 42.5.2 - [#30755](quarkusio/quarkus#30755) - Update ForwardedParser to validate the port - [#30744](quarkusio/quarkus#30744) - \[Quarkus Native] ClassNotFoundException: com.github.benmanes.caffeine.cache.SSSW - [#30536](quarkusio/quarkus#30536) - munitnyucontextmanager non helpful error reporting - [#29753](quarkusio/quarkus#29753) - Introduce ConnectionFactoryWrapperBuildItem - [#29605](quarkusio/quarkus#29605) - Update docs to reflect that injection should not - [#27774](quarkusio/quarkus#27774) - PLANNER-1709 Avoid deprecated penalize/reward overloads - [#23442](quarkusio/quarkus#23442) - problem using quarkus-resteasy-reactive-kotlin-serialization with AwsProxyRequestContext </details> <details> <summary>quarkusio/quarkus-platform</summary> ### [`v2.16.2.Final`](quarkusio/quarkus-platform@2.16.1.Final...2.16.2.Final) [Compare Source](quarkusio/quarkus-platform@2.16.1.Final...2.16.2.Final) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever MR is behind base branch, or you tick the rebase/retry checkbox. 👻 **Immortal**: This MR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

quarkus-bot bot added the area/core label Dec 8, 2022

turing85 reviewed Dec 8, 2022

View reviewed changes

mkouba reviewed Dec 8, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

zhfeng force-pushed the add_connection_factory_wrap branch from 5396fe9 to 84a9a24 Compare January 3, 2023 08:07

quarkus-bot bot added the area/dependencies Pull requests that update a dependency file label Jan 3, 2023

This comment has been minimized.

Sign in to view

zhfeng force-pushed the add_connection_factory_wrap branch from 84a9a24 to 659d328 Compare January 31, 2023 13:42

This comment has been minimized.

Sign in to view

gsmet reviewed Feb 2, 2023

View reviewed changes

gsmet requested changes Feb 2, 2023

View reviewed changes

...eployment/src/main/java/io/quarkus/jms/spi/deployment/ConnectionFactoryWrapperBuildItem.java Outdated Show resolved Hide resolved

zhfeng force-pushed the add_connection_factory_wrap branch 3 times, most recently from f47c930 to 28a1026 Compare February 2, 2023 09:57

zhfeng requested a review from gsmet February 2, 2023 09:59

gsmet reviewed Feb 2, 2023

View reviewed changes

gsmet approved these changes Feb 2, 2023

View reviewed changes

gsmet added the triage/quarkus-3 label Feb 2, 2023

This comment has been minimized.

Sign in to view

gsmet added the triage/needs-rebase This PR needs to be rebased first because it has merge conflicts label Feb 4, 2023

zhfeng added 2 commits February 6, 2023 19:57

Introduce jms-spi with ConnectionFactoryWrapperBuildItem

cda8d9e

Update jakarta jms-api

7049748

zhfeng force-pushed the add_connection_factory_wrap branch from 28a1026 to 7049748 Compare February 6, 2023 12:23

gsmet approved these changes Feb 6, 2023

View reviewed changes

gsmet removed triage/needs-rebase This PR needs to be rebased first because it has merge conflicts triage/quarkus-3 labels Feb 6, 2023

gsmet added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Feb 6, 2023

gsmet merged commit 5928de0 into quarkusio:main Feb 7, 2023

quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Feb 7, 2023

quarkus-bot bot added this to the 3.0 - main milestone Feb 7, 2023

gsmet added the triage/backport? label Feb 7, 2023

gsmet modified the milestones: 3.0 - main, 2.16.2.Final Feb 7, 2023

gsmet removed the triage/backport? label Feb 7, 2023

This was referenced Feb 10, 2023

Use ConnectionFactoryWrapperBuildItem in Quarkus quarkiverse/quarkus-pooled-jms#71

Closed

Use ConnectionFactoryWrapperBuildItem in Quarkus quarkiverse/quarkus-artemis#148

Closed

zhfeng mentioned this pull request Mar 1, 2023

Add pooling support amqphub/quarkus-qpid-jms#22

Closed

zhfeng mentioned this pull request Apr 20, 2023

Fix #22 to add pooling support amqphub/quarkus-qpid-jms#23

Merged

zhfeng mentioned this pull request Sep 9, 2024

Consider adding support of InterceptionFactory #3532

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce ConnectionFactoryWrapperBuildItem #29753

Introduce ConnectionFactoryWrapperBuildItem #29753

zhfeng commented Dec 8, 2022

turing85 Dec 8, 2022 •

edited

Loading

mkouba Dec 8, 2022

turing85 Dec 8, 2022

zhfeng Dec 8, 2022

mkouba Dec 8, 2022

zhfeng Dec 8, 2022

mkouba Dec 8, 2022

zhfeng Dec 8, 2022

mkouba Dec 8, 2022

zhfeng Dec 8, 2022

mkouba Dec 8, 2022

n1hility Jan 30, 2023

zhfeng Jan 31, 2023

zhfeng Feb 1, 2023

gsmet Feb 2, 2023

zhfeng Feb 2, 2023

This comment has been minimized.

This comment has been minimized.

zhfeng commented Jan 30, 2023

mkouba commented Jan 30, 2023

zhfeng commented Jan 30, 2023

mkouba commented Jan 30, 2023

This comment has been minimized.

gsmet Feb 2, 2023

zhfeng Feb 2, 2023

gsmet Feb 2, 2023

gsmet left a comment

gsmet Feb 2, 2023

zhfeng Feb 2, 2023

gsmet left a comment

This comment has been minimized.

quarkus-bot bot commented Feb 6, 2023

gsmet commented Feb 7, 2023

zhfeng commented Feb 7, 2023


		import javax.jms.ConnectionFactory;

		import org.wildfly.common.Assert;

Introduce ConnectionFactoryWrapperBuildItem #29753

Introduce ConnectionFactoryWrapperBuildItem #29753

Conversation

zhfeng commented Dec 8, 2022

turing85 Dec 8, 2022 • 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

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

This comment has been minimized.

This comment has been minimized.

zhfeng commented Jan 30, 2023

mkouba commented Jan 30, 2023

zhfeng commented Jan 30, 2023

mkouba commented Jan 30, 2023

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsmet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsmet left a comment

Choose a reason for hiding this comment

This comment has been minimized.

quarkus-bot bot commented Feb 6, 2023

gsmet commented Feb 7, 2023

zhfeng commented Feb 7, 2023

turing85 Dec 8, 2022 •

edited

Loading