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.

Sign up for GitHub

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

Jump to bottom

Add Artemis Resource Adapter support #341

Merged
merged 3 commits into from
Dec 2, 2023
Merged

Add Artemis Resource Adapter support #341

merged 3 commits into from
Dec 2, 2023

Conversation

zhfeng
Copy link
Contributor

@zhfeng zhfeng commented Nov 29, 2023 •
edited
Loading

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@zhfeng zhfeng mentioned this pull request Nov 29, 2023
Closed
gastaldi
gastaldi approved these changes Nov 29, 2023
View reviewed changes
Copy link
Member

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

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

LGTM, but I think @vsevel might have some changes to add to the ArtemisResourceAdapterFactory to meet his requirements, so better wait for his feedback before merging or incorporating this in the next release

@zhfeng
Copy link
Contributor Author

zhfeng commented Nov 29, 2023

Sure, I'm still working on some docs.

@vsevel
Copy link

vsevel commented Nov 30, 2023

hello @gastaldi and @zhfeng nice addition!
I checked that the ArtemisResourceAdapterFactory has the options protocol-manager-factory and rebalance-connection, that is good.
one thing I added in my implementation, is a way to wrap the message listener in a custom class. in my situation I just hardcoded the common behavior: reading some common headers and adding them to a custom request context.
there was also a question on support for duplicated context. right now I am storing this request context in a request scope bean for jms messages, and into the duplicated context for everything else.

@zhfeng
Copy link
Contributor Author

zhfeng commented Nov 30, 2023

Thanks @vsevel and I wonder if ther is a chance to add a test for rebalance-connection? How to setup an Artemis Cluster for testing?

turing85
turing85 reviewed Nov 30, 2023
View reviewed changes
Copy link
Contributor

@turing85 turing85 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

There are still some open questions I have. Some are mentioned in my review comment. I think it currently boils down to the fact that we are missing documentation for this feature.

Major questions I have:

  • How does quarkus-ironjacamar interact with pooled-jms? Do they interact? Do they interfer?
  • I see that an XAConnectionFactory is provided by the extension. Does XA "just work"?

One major remark I have: there are still some things commented-out (mainly in the application.properties of the integration test). Can we remove them?

If I read the code correctly, it seems like we are missing support for devservices. I think that this should be feasible by iterating over all ironjacamar configurations ans look for the ones with kind="Artemis". This should probably be a separate issue.

ra/runtime/pom.xml Show resolved Hide resolved
integration-tests/ra/src/test/java/io/quarkus/it/artemis/ra/JcaResourceTest.java Outdated Show resolved Hide resolved
integration-tests/ra/src/main/java/io/quarkus/it/artemis/ra/MyQueueMessageEndpoint.java Show resolved Hide resolved
integration-tests/ra/src/main/resources/application.properties Outdated Show resolved Hide resolved
bom/pom.xml Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
integration-tests/ra/src/test/java/io/quarkus/it/artemis/ra/MetricsTest.java Show resolved Hide resolved
integration-tests/ra/src/test/java/io/quarkus/it/artemis/ra/TransactionTest.java Outdated Show resolved Hide resolved
@gastaldi
Copy link
Member

There are still some open questions I have. Some are mentioned in my review comment. I think it currently boils down to the fact that we are missing documentation for this feature.

Major questions I have:

  • How does quarkus-ironjacamar interact with pooled-jms? Do they interact? Do they interfer?

I believe they are different things. quarkus-ironjacamar allows running a JCA resource adapter in Quarkus, enabling features implemented in these adapters, such as load-balancing and transaction enlistment (eg. if you rollback a transaction in a method where a message is consumed, the message is not consumed and may be moved to a DLQ).

  • I see that an XAConnectionFactory is provided by the extension. Does XA "just work"?

That's up to the Resource adapter implementation. The ironjacamar extension enlists them in a similar way as it's done in WildFly, so it should just work™

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@zhfeng zhfeng force-pushed the feature/add_artemis_jms_ra branch from 27d8a07 to 2b41ba0 Compare November 30, 2023 14:29
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@zhfeng zhfeng force-pushed the feature/add_artemis_jms_ra branch from 2b41ba0 to dc7c70c Compare December 1, 2023 02:19
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@vsevel
Copy link

vsevel commented Dec 1, 2023

Thanks @vsevel and I wonder if ther is a chance to add a test for rebalance-connection? How to setup an Artemis Cluster for testing?

you need to start a cluster, connect, shut down one server, restart the stopped server, make sure you have connections on both servers again.

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@zhfeng
Copy link
Contributor Author

zhfeng commented Dec 1, 2023

@vsevel is there any way to start a MQ cluster by using docker?

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
turing85
turing85 requested changes Dec 1, 2023
View reviewed changes
integration-tests/ra/src/test/java/io/quarkus/it/artemis/ra/TransactionTest.java
public void retryMessagesOnRollback() {
// @formatter:off
RestAssured
.given()
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the empty .given()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I think it is needes here because there is "formParm(...)".

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh true!
Could you either pull formParam(...) up (preferred) or indent it?

...eployment/src/main/java/io/quarkus/artemis/core/deployment/ArtemisBootstrappedBuildItem.java Outdated Show resolved Hide resolved
...ployment/src/main/java/io/quarkus/artemis/core/deployment/health/ArtemisHealthProcessor.java Outdated Show resolved Hide resolved
...rc/main/java/io/quarkus/artemis/core/deployment/health/ExtraArtemisHealthCheckBuildItem.java Show resolved Hide resolved
integration-tests/jms/with-default-and-external/src/main/resources/application.properties Show resolved Hide resolved
integration-tests/ra/src/test/java/io/quarkus/it/artemis/ra/DisableAllServices.java Outdated Show resolved Hide resolved
integration-tests/ra/src/test/java/io/quarkus/it/artemis/ra/DisableDataBaseService.java Outdated Show resolved Hide resolved
@zhfeng
Copy link
Contributor Author

zhfeng commented Dec 1, 2023

The native build failure is related to

gastaldi
gastaldi reviewed Dec 1, 2023
View reviewed changes
pom.xml Outdated Show resolved Hide resolved
@turing85 turing85 added enhancement New feature or request backport/3.0.x labels Dec 1, 2023
@zhfeng zhfeng force-pushed the feature/add_artemis_jms_ra branch from b04df1b to 6e2e369 Compare December 1, 2023 15:43
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@zhfeng zhfeng force-pushed the feature/add_artemis_jms_ra branch from 6e2e369 to 545d436 Compare December 2, 2023 04:37
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@zhfeng zhfeng force-pushed the feature/add_artemis_jms_ra branch from 545d436 to d90f72b Compare December 2, 2023 04:44
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 2, 2023 •
edited
Loading

🚦Reports for run #854🚦

Reports will be posted here as they get available.

🥳 JUnit JVM Test passed
Passed Failed Skipped
✅ 181 ❌ 0 ⚠️ 0

You can see the report here.

🥳 JUnit Native Test passed
Passed Failed Skipped
✅ 113 ❌ 0 ⚠️ 0

You can see the report here.

@zhfeng zhfeng merged commit 35a1ab9 into main Dec 2, 2023
43 checks passed
@zhfeng zhfeng deleted the feature/add_artemis_jms_ra branch December 2, 2023 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gastaldi gastaldi gastaldi approved these changes

@middagj middagj middagj approved these changes

@turing85 turing85 turing85 approved these changes

@vsevel vsevel Awaiting requested review from vsevel

Assignees

@zhfeng zhfeng

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support Rebalancing connections
5 participants
@zhfeng @vsevel @gastaldi @middagj @turing85