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

[WFLY-15405] AMQP Connector for reactive messaging #16281

Merged
merged 3 commits into from Dec 15, 2023

Conversation

kabir
Copy link
Contributor

@kabir kabir commented Nov 14, 2022

https://issues.redhat.com/browse/WFLY-15405 - Support for the AMQP connector in the MP Reactive Messaging subsystem

Includes #17343 so this can be tested with the latest SmallRye Reactive Messaging which will likely be merged by the time this is tested

@kabir kabir added Feature PR provides a new feature missing-reqs Features missing one or more of the pre-merge requirements labels Nov 14, 2022
@bstansberry
Copy link
Contributor

@kabir The weld/transaction commit can be rebased out; that JIRA is resolved in main.

@kabir
Copy link
Contributor Author

kabir commented Nov 15, 2022

@bstansberry oops. Should be good now

@github-actions github-actions bot added the deps-changed Dependencies have been checked, and there are changes highlighted in a comment label Nov 15, 2022
@github-actions
Copy link

github-actions bot commented Nov 15, 2022

Dependency Tree Analyzer Output:

New Dependencies:

  • io.smallrye.reactive:smallrye-mutiny-vertx-amqp-client:jar:3.6.0:compile
  • io.smallrye.reactive:smallrye-reactive-messaging-amqp:jar:4.11.0:compile
  • io.vertx:vertx-amqp-client:jar:4.4.6:compile
  • io.vertx:vertx-proton:jar:4.4.6:compile
  • org.wildfly:wildfly-microprofile-reactive-messaging-amqp:jar:31.0.0.Beta1-SNAPSHOT:compile

CC @wildfly/prod

@kabir kabir changed the title Reactive amqp 15405 [WFLY-15405] AMQP Connector for reactive messaging Jan 10, 2023
Copy link
Contributor

@jstourac jstourac left a comment

Choose a reason for hiding this comment

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

I've went through circa half of this PR and put some comments. Mostly regarding the documentation. I'll do rest later :)

@kabir kabir force-pushed the reactive-amqp-15405 branch 3 times, most recently from 477b3f8 to 7589e86 Compare May 22, 2023 13:02
Copy link
Contributor

@jstourac jstourac left a comment

Choose a reason for hiding this comment

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

Hi Kabir, I've put two more minor comments regarding the documentation. I believe that this is all from me at the moment. Note there is one comment from me from last week that you didn't address. Is it on purpose or you just missed it/postponed it? 🙂


`mp.messaging.outgoing.to.connector=smallrye-amqp` says that we want to use AMQP to back the `to` channel. Note that the value `smallrye-amqp` is SmallRye Reactive Messaging specific, and will only be understood if the AMQP connector is enabled.

`mp.messaging.outgoing.to.address=my-queue` says that we will send data via `to` channel to the AMQP queue on address called `my-queue`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`mp.messaging.outgoing.to.address=my-queue` says that we will send data via `to` channel to the AMQP queue on address called `my-queue`.
`mp.messaging.outgoing.to.address=my-topic` says that we will send data via `to` channel to the AMQP topic (or queue) on address called `my-topic`.

Just to stay consistent with the above example.


`mp.messaging.incoming.from.connector=smallrye-amqp` says that we want to use AMQP to back the `from` channel. As above, the value `smallrye-amqp` is SmallRye Reactive Messaging specific.

`mp.messaging.incoming.from.address=my-queue` says says that the channel named `from` will read data from the AMQP topic (or queue) on address called `my-topic`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`mp.messaging.incoming.from.address=my-queue` says says that the channel named `from` will read data from the AMQP topic (or queue) on address called `my-topic`.
`mp.messaging.incoming.from.address=my-topic` says that the channel named `from` will read data from the AMQP topic (or queue) on address called `my-topic`.

@kabir
Copy link
Contributor Author

kabir commented May 23, 2023

@jstourac I missed it. Hopefully it should be fine now :-)

@kabir kabir removed the rebase-this PR has a merge conflict. label Nov 10, 2023
@wildfly-bot wildfly-bot bot added the rebase-this PR has a merge conflict. label Nov 10, 2023
@kabir kabir force-pushed the reactive-amqp-15405 branch 3 times, most recently from d7ad79d to 35a71f7 Compare November 17, 2023 16:16
@kabir kabir force-pushed the reactive-amqp-15405 branch 2 times, most recently from 8a57a4c to 7b2cf2a Compare December 8, 2023 16:48
@wildfly-bot wildfly-bot bot removed the rebase-this PR has a merge conflict. label Dec 8, 2023
@kabir kabir force-pushed the reactive-amqp-15405 branch 4 times, most recently from 31cfb8d to f859204 Compare December 11, 2023 12:57
@mnovak1
Copy link
Contributor

mnovak1 commented Dec 12, 2023

FYI: I've run Wildfly TS with latest changes and there is no issue related to this PR.

/*
* Copyright The WildFly Authors
* SPDX-License-Identifier: Apache-2.0
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

This one has two headers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the second one

@@ -0,0 +1,57 @@
/*
* JBoss, Home of Professional Open Source.
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be ASL now?

Copy link
Contributor

Choose a reason for hiding this comment

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

See L24-27 -- it looks like the header switch got messed up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bstansberry already fixed?

@@ -0,0 +1,74 @@
/*
* JBoss, Home of Professional Open Source.
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be ASL now?

Next we will briefly discuss each of these entries. Remember the `to` channel is on the `send()` method, and the `from` channel is on the `receive()` method.


'amqp-host=localhost' '`amqp-port=5672` points the connector to an AMQP broker running on `localhost:5672`. As before we could also have done these for an individual channel by for example specifying `mp.messaging.outgoing.to.host=localhost` instead. If the host is not specified, it defaults to `localhost.
Copy link
Contributor

Choose a reason for hiding this comment

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

@kabir This can be done in a follow up fix, but whatever turns on italics here needs to be fixed.

Note -- please don't make any docs fixes I mention here on this PR unless you have to change something that justifies new CI. They can come later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bstansberry I've never seen the source be italic before, but it turns out it was due to some unclosed quotes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in #17491

<dependency>
<groupId>io.smallrye.reactive</groupId>
<artifactId>smallrye-mutiny-vertx-amqp-client</artifactId>
<exclusions>
Copy link
Contributor

Choose a reason for hiding this comment

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

Future fix:

'exclusions' should not appear here unless there is a good reason they don't appear in the bom.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAICR those boms did not exist when I worked on this code 1.5 years ago :-D

I'll leave it as-is, and have opened https://issues.redhat.com/browse/WFLY-18852 to clean up this pom since I see other exclusions than mine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made a start in #17491

@@ -705,6 +705,7 @@
<layer>microprofile-openapi</layer>
<layer>microprofile-reactive-streams-operators</layer>
<layer>microprofile-reactive-messaging</layer>
<layer>microprofile-reactive-messaging-amqp</layer>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please file a JIRA to add a provisioning execution with only the layer. All layers need an individual execution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in #17491

@bstansberry bstansberry merged commit df398be into wildfly:main Dec 15, 2023
15 checks passed
@bstansberry
Copy link
Contributor

Thanks @kabir!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deps-changed Dependencies have been checked, and there are changes highlighted in a comment Feature PR provides a new feature missing-reqs Features missing one or more of the pre-merge requirements
Projects
None yet
5 participants