Add support for RabbitMQ AMQP 1.0 #46608
Conversation
Signed-off-by: Eddú Meléndez <[email protected]>
Signed-off-by: Eddú Meléndez <[email protected]>
eddumelendez
force-pushed
the
rabbitmq-amqp
branch
from
77d69d9 to
02ba888
Compare
spring-projects-issues
added
the
status: waiting-for-triage
Signed-off-by: Eddú Meléndez <[email protected]>
eddumelendez
changed the title
...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.
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.
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.
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 <[email protected]>
eddumelendez
force-pushed
the
rabbitmq-amqp
branch
from
4c79b2e to
6572c36
Compare
Signed-off-by: Eddú Meléndez <[email protected]>
...p/src/main/java/org/springframework/boot/amqp/autoconfigure/RabbitAmqpAutoConfiguration.java
Show resolved
Hide resolved
Signed-off-by: Eddú Meléndez <[email protected]>
Signed-off-by: Eddú Meléndez <[email protected]>
Signed-off-by: Eddú Meléndez <[email protected]>
Signed-off-by: Eddú Meléndez <[email protected]>
| * @since 4.0.0 | ||
| */ | ||
| @AutoConfiguration | ||
| @ConditionalOnClass({ RabbitTemplate.class, Channel.class }) | ||
| @ConditionalOnMissingClass({ "com.rabbitmq.client.amqp.Connection", | ||
| "org.springframework.amqp.rabbitmq.client.RabbitAmqpTemplate" }) |
There was a problem hiding this comment.
I don't think this is correct.
We may still have those classes on classpath, but opt-in to use AMQP 0.9.1.
For example, via auto-configuration exclusion for that our new RabbitAmqpAutoConfiguration.
Why not conditional on bean?
| */ | ||
| @ExtendWith(OutputCaptureExtension.class) | ||
| class RabbitAutoConfigurationTests { | ||
|
|
||
| private final ApplicationContextRunner contextRunner = new ApplicationContextRunner() | ||
| .withConfiguration(AutoConfigurations.of(RabbitAutoConfiguration.class, SslAutoConfiguration.class)) | ||
| .withClassLoader(new FilteredClassLoader("org.springframework.rabbit.stream")); // gh-38750 | ||
| .withClassLoader(new FilteredClassLoader("org.springframework.rabbit.stream", "com.rabbitmq.client.amqp", | ||
| "org.springframework.amqp.rabbitmq.client")); // gh-38750 |
There was a problem hiding this comment.
I don't think a GH issue is relevant to your change.
Plus, I see you do the same in other place, but no GH issue number for whatsoever...
| .withPropertyValues("management.metrics.use-global-registry=false"); | ||
| .withPropertyValues("management.metrics.use-global-registry=false") | ||
| .withClassLoader( | ||
| new FilteredClassLoader("com.rabbitmq.client.amqp", "org.springframework.amqp.rabbitmq.client")); |
There was a problem hiding this comment.
I think if you make a RabbitAutoConfiguration conditional on bean from our new AMQP 1.0 auto-configuration, then we would not need this kind of classloader filtering.
Since we just don't include that RabbitAmqpAutoConfiguration into these existing tests.
| id "org.springframework.boot.starter" | ||
| } | ||
|
|
||
| description = "Starter for using Spring AMQP and Rabbit MQ" |
There was a problem hiding this comment.
Can we be more specific, e.g. : Starter for using Spring AMQP with Rabbit MQ over AMQP 0.9.1 protocol?
And probably something similar in existing started to emphasize that it is about AMQP 1.0 protocol.
Uh oh!
There was an error while loading. Please reload this page.