Add support for RabbitMQ AMQP 1.0#46608
Add support for RabbitMQ AMQP 1.0#46608eddumelendez wants to merge 12 commits intospring-projects:mainfrom
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>
...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
smoke-test/spring-boot-smoke-test-rabbit-amqp/src/main/java/smoketest/amqp/Sender.java
Show resolved
Hide resolved
| * @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 <eddu.melendez@gmail.com>
4c79b2e to
6572c36
Compare
Signed-off-by: Eddú Meléndez <eddu.melendez@gmail.com>
...p/src/main/java/org/springframework/boot/amqp/autoconfigure/RabbitAmqpAutoConfiguration.java
Show resolved
Hide resolved
Signed-off-by: Eddú Meléndez <eddu.melendez@gmail.com>
Signed-off-by: Eddú Meléndez <eddu.melendez@gmail.com>
Signed-off-by: Eddú Meléndez <eddu.melendez@gmail.com>
Signed-off-by: Eddú Meléndez <eddu.melendez@gmail.com>
| @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?
There was a problem hiding this comment.
Switched to conditional on missing bean
.../src/test/java/org/springframework/boot/amqp/autoconfigure/RabbitAutoConfigurationTests.java
Outdated
Show resolved
Hide resolved
...org/springframework/boot/amqp/autoconfigure/metrics/RabbitMetricsAutoConfigurationTests.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Eddú Meléndez <eddu.melendez@gmail.com>
05bec25 to
2be167a
Compare
Signed-off-by: Eddú Meléndez <eddu.melendez@gmail.com>
2be167a to
b3b1d45
Compare
Signed-off-by: Eddú Meléndez <eddu.melendez@gmail.com>
artembilan
left a comment
There was a problem hiding this comment.
LGTM.
Might be some docs also required, but so far this is great.
Thank you!
|
@artembilan and I brainstormed a bit and things should be in a different state than what's been discussed thus far:
Things are a bit blurry at this point given that we can merge this in SB 4.1 at the earliest. By then, Spring AMQP may have support for other brokers so that suggested name above wouldn't work. I've flagged this for team meeting to see what the team thinks. If we want to go with two separate modules, then we might rename the current one |
|
@eddumelendez this is what the conclusion we came to in preparation for the next version of Spring AMQP:
Auto-configuration of both a 0.9 client and a 1.0 client is possible if you add the two modules on the classpath (contrary to what's been discussed during the review). Do you want to explore this option and amend your PR? If not, no worries and please let me know either way. |
|
Thanks for sharing, @snicoll! I'll work on this changes |
|
Heya @eddumelendez, any update for us? If you're still working on it, no worries. I just want to avoid adding a change like that in the RC. |
|
Hi @snicoll, yes, still working on it. Should have an update next week. |
Uh oh!
There was an error while loading. Please reload this page.