-
Notifications
You must be signed in to change notification settings - Fork 41.4k
Add support for RabbitMQ AMQP 1.0 #46608
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Eddú Meléndez <eddu.melendez@gmail.com>
Signed-off-by: Eddú Meléndez <eddu.melendez@gmail.com>
77d69d9
to
02ba888
Compare
Signed-off-by: Eddú Meléndez <eddu.melendez@gmail.com>
@@ -41,6 +41,7 @@ dependencies { | |||
optional(project(":module:spring-boot-metrics")) | |||
optional("io.micrometer:micrometer-core") | |||
optional("org.springframework.amqp:spring-rabbit-stream") | |||
optional("org.springframework.amqp:spring-rabbitmq-client") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better to have an AMQP 1.0 as default one.
Therefore I suggest to do api("org.springframework.amqp:spring-rabbitmq-client")
instead of that api("org.springframework.amqp:spring-rabbit")
.
The spring-rabbitmq-client
has spring-rabbit
as dependency.
So, both AMQP 1.0 and 0.9.1 are on classpath.
but that should be end-user choice to opt-in to 0.9.1
by excluding spring-rabbitmq-client
auto-configuration or so.
I mean this is Spring Boot 4.0 and we probably can bite a bullet and make AMQP 1.0 auto-configured by default.
Just my opinion: we can continue discussion.
...p/src/main/java/org/springframework/boot/amqp/autoconfigure/RabbitAmqpAutoConfiguration.java
Outdated
Show resolved
Hide resolved
...p/src/main/java/org/springframework/boot/amqp/autoconfigure/RabbitAmqpAutoConfiguration.java
Outdated
Show resolved
Hide resolved
private RabbitAmqpTemplate rabbitAmqpTemplate; | ||
|
||
public void send(String message) { | ||
this.rabbitAmqpTemplate.convertAndSend("foo", message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is foo/bar
language OK with Spring Boot team?
* @author Eddú Meléndez | ||
* @since 4.0.0 | ||
*/ | ||
@AutoConfiguration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think need to make before = RabbitAutoConfiguration.class
and make that one conditional on something missing from this RabbitAmqpAutoConfiguration
.
This is an addition to my previous point: the AMQP 1.0 is by default.
If we don't make them conditional on each other, then both protocols are going to be enabled and connected.
Especially, I worry about RabbitAmqpAdmin
and RabbitAdmin
which are going to handle topology in parallel but in different connections.
I don't think that we need to give a choice to be able to have both protocols in one Spring Boot application.
|
||
private final RabbitProperties properties; | ||
|
||
RabbitAmqpAutoConfiguration(RabbitProperties properties) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to look into a separate RabbitAmqpProperties
abstraction.
Even if some duplication is possible, a bunch of existing properties are not going to be used for AMQP 1.0 at all.
And might cause confusion.
Signed-off-by: Eddú Meléndez <eddu.melendez@gmail.com>
4c79b2e
to
6572c36
Compare
Signed-off-by: Eddú Meléndez <eddu.melendez@gmail.com>
Uh oh!
There was an error while loading. Please reload this page.