-
Notifications
You must be signed in to change notification settings - Fork 636
GH-1419: Remove RabbitMQ http-client Usage #1511
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
Conversation
Resolves spring-projects#1419 Use Spring WebFlux instead, while allowing the user to choose some other technology in the `LocalizedQueueConnectionFactory`.
5b801c8
to
8000261
Compare
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.
Just a couple questions.
And most important the reason behind back-porting this fix...
build.gradle
Outdated
optionalApi 'org.springframework:spring-aop' | ||
api 'org.springframework:spring-context' | ||
api 'org.springframework:spring-messaging' | ||
api 'org.springframework:spring-tx' | ||
optionalApi 'org.springframework:spring-web' |
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.
This one comes as a transitive dep from WebFlux: api(project(":spring-web"))
.
So, why to use it explicitly?
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.
Not sure; there was some class in it that wasn't on the CP until I added this; will try to remove.
build.gradle
Outdated
@@ -410,7 +410,7 @@ project('spring-rabbit') { | |||
testImplementation 'io.micrometer:micrometer-tracing-test' | |||
testImplementation 'io.micrometer:micrometer-tracing-integration-test' | |||
testRuntimeOnly 'org.springframework:spring-web' | |||
testRuntimeOnly "org.apache.httpcomponents:httpclient:$commonsHttpClientVersion" | |||
testRuntimeOnly "org.apache.httpcomponents.client5:httpclient5:$commonsHttpClientVersion" |
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.
Why doesn't reactor-netty-http
works here for us?
I mean it is still OK since it is a test dep, but probably with Reactor in hands we may avoid and extra dep to manage.
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 will try with that.
* Used to obtain a connection factory for the queue leader. | ||
* | ||
* @param <T> the client type. | ||
* @since 2.4.8 |
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.
Why have you decided to back-port this fix?
Do you have an evidence that Spring Framework 5.3.x has moved to Commons Http Client 5.0?
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.
My plan is to back port but with the default NodeLocator
being a RabbitMQHopNodeLocator
(immediately deprecated) and make the WebFluxNodeLocator
an optional component.
See the issue; the problem is HOP will be out of support before 2.4.x is, so we will need an option in 2.4.x that doesn't depend on HOP, in case of future REST changes in RabbitMQ.
I'd recommend going with Reactor Netty, you'll pull fewer dependencies. And IIRC Spring AMQP uses only 1 REST endpoint, so its needs are limited. Note Hop uses Reactor Netty as well if you want some inspiration. We had a discussion years ago and the gist was that using |
Resolves #1419
Use Spring WebFlux instead, while allowing the user to choose some other technology in the
LocalizedQueueConnectionFactory
.I will backport after review/merge