GH-9228: ZeroMQ - provide binding to ZeroMqMessageHandler #9229

alessiomatricardi · 2024-06-11T00:00:23Z

This PR provides the bind capability to the outbound channel adapter ZeroMqMessageHandler.

Consider to implement that logic in the Lifecycle.start()

I tried, but the current implementation instantiate the socket within the class constructor, and some old tests relies on this mechanism.

artembilan

Please, also add your name to the @author list of all the affected classes.
And verify changes locally like ./gradlew :spring-integration-zeromq:check.

src/reference/antora/modules/ROOT/pages/zeromq.adoc

artembilan · 2024-06-11T17:49:53Z

...eromq/src/main/java/org/springframework/integration/zeromq/dsl/ZeroMqMessageHandlerSpec.java

+	 * Create an instance based on the provided {@link ZContext} and binding port.
+	 * @param context the {@link ZContext} to use for creating sockets.
+	 * @param port the port to bind ZeroMq socket to over TCP.
+	 * @since 6.2.6


As I said in the issue it is something new and will be available only starting with 6.4.

artembilan · 2024-06-11T17:50:48Z

...eromq/src/main/java/org/springframework/integration/zeromq/dsl/ZeroMqMessageHandlerSpec.java

+	 * @param port the port to bind ZeroMq socket to over TCP.
+	 * @since 6.2.6
+	 */
+	public ZeroMqMessageHandlerSpec(ZContext context, int port) {


These constructors have to be protected and respective factories have to be introduced into org.springframework.integration.zeromq.dsl.ZeroMq class.

See cecc3c5

artembilan · 2024-06-11T17:54:04Z

...romq/src/main/java/org/springframework/integration/zeromq/outbound/ZeroMqMessageHandler.java

+				Mono.just(this.context.createSocket(this.socketType))
+						.publishOn(this.publisherScheduler)
+						.doOnNext((socket) -> this.socketConfigurer.accept(socket))
+						.doOnNext((socket) -> this.bindPort.set(bindSocket(socket, this.bindPort.get())))


The binding has to happen in the start().
If there are some failing tests according to a new behavior, then exactly they have to be fixed.
Probably just calling that start() in their logic.

Well, actually we do this in the start():

this.socketMonoSubscriber = this.socketMono.subscribe();

So, all good, however looking into this one more time I believe that we have to always renew this socketMono in the start(). After the stop() the environment might be changed, so or now way to connect or bind.
Therefore every time when we start this component we have to renew socket.

I'm also thinking about DRY principle here.
Looks like we now have two socket Mono which has difference only in these lines:

.doOnNext((socket) -> socket.connect(connectUrl.get()))

in opposed to:

.doOnNext((socket) -> this.bindPort.set(bindSocket(socket, this.bindPort.get())))

Essentially if..else according to the server/client preferences.
This could be just extracted into a private method where we would do connect or bind.
And monoSocket would be just a single instance in the start without any duplication.

WDYT?

Completely agree with you. I was trying to touch less code as possible, but as you said this is the best approach

artembilan

Also this new functionality has to be mention in the whats-new.adoc. Like a new section in the end:

[[x6.4-zeromq-changes]]
=== ZeroMQ Changes

artembilan · 2024-06-12T16:45:16Z

...romq/src/main/java/org/springframework/integration/zeromq/outbound/ZeroMqMessageHandler.java

+				Mono.just(this.context.createSocket(this.socketType))
+						.publishOn(this.publisherScheduler)
+						.doOnNext((socket) -> this.socketConfigurer.accept(socket))
+						.doOnNext((socket) -> this.bindPort.set(bindSocket(socket, this.bindPort.get())))


Well, actually we do this in the start():

this.socketMonoSubscriber = this.socketMono.subscribe();

So, all good, however looking into this one more time I believe that we have to always renew this socketMono in the start(). After the stop() the environment might be changed, so or now way to connect or bind.
Therefore every time when we start this component we have to renew socket.

I'm also thinking about DRY principle here.
Looks like we now have two socket Mono which has difference only in these lines:

.doOnNext((socket) -> socket.connect(connectUrl.get()))

in opposed to:

.doOnNext((socket) -> this.bindPort.set(bindSocket(socket, this.bindPort.get())))

Essentially if..else according to the server/client preferences.
This could be just extracted into a private method where we would do connect or bind.
And monoSocket would be just a single instance in the start without any duplication.

WDYT?

…roMq

…rs and simple constructors, following the same logic used for ZeroMqMessageProvider

alessiomatricardi · 2024-06-17T20:52:22Z

Addressed almost every comment. I also added a typo in ZeroMqHeaders, but I've not added myself as author because no code has been touched. Let me know if I should do it

artembilan · 2024-06-18T14:34:24Z

Thank you for update!
I'll come back to you with review later this week.
Today is a release day for patch versions.
This new feature will make it only into the next 6.4 one.

artembilan

This is great!
Just couple follow up suggestions.

Thank you!

spring-integration-zeromq/src/main/java/org/springframework/integration/zeromq/ZeroMqUtils.java

...romq/src/main/java/org/springframework/integration/zeromq/outbound/ZeroMqMessageHandler.java

artembilan · 2024-06-20T16:09:02Z

spring-integration-zeromq/src/main/java/org/springframework/integration/zeromq/dsl/ZeroMq.java

+	 * @return the spec.
+	 * @since 6.4
+	 */
+	public static ZeroMqMessageHandlerSpec outboundChannelAdapter(ZContext context, int port) {


I wonder if DSL has to expose extra factories to explicitly bind to random port.
Something similar what you have with those new ctors in the ZeroMqMessageHandler.

Or you have missed to push the change, or just didn't implement yet.
Hence the conversion is not resolved yet 😄

alessiomatricardi · 2024-06-20T17:17:35Z

Addressed your comments!

artembilan

Nice progress!
I believe we are very close for merge.
Please, take a look to the latest review.
Honestly, nothing critical and I can address them on merge.
Just let me know your preferences!

spring-integration-zeromq/src/main/java/org/springframework/integration/zeromq/ZeroMqUtils.java

...eromq/src/main/java/org/springframework/integration/zeromq/dsl/ZeroMqMessageHandlerSpec.java

artembilan · 2024-06-20T19:22:54Z

spring-integration-zeromq/src/main/java/org/springframework/integration/zeromq/dsl/ZeroMq.java

+	 * @return the spec.
+	 * @since 6.4
+	 */
+	public static ZeroMqMessageHandlerSpec outboundChannelAdapter(ZContext context, int port) {


Or you have missed to push the change, or just didn't implement yet.
Hence the conversion is not resolved yet 😄

…et will be bound to a random port

alessiomatricardi · 2024-06-29T16:23:16Z

Hi, sorry for the delay but I'm very busy with work.. hope now looks good for you :)

artembilan · 2024-07-01T17:00:55Z

@alessiomatricardi ,

thank you very much for the contribution; looking forward for more!

alessiomatricardi added 2 commits June 11, 2024 01:52

first implementation

b2f83e3

add docs

4630c86

alessiomatricardi marked this pull request as ready for review June 11, 2024 08:29

artembilan requested changes Jun 11, 2024

View reviewed changes

artembilan requested changes Jun 12, 2024

View reviewed changes

alessiomatricardi added 7 commits June 17, 2024 22:42

protected constructor in ZeroMqMessageHandlerSpec, expose them via Ze…

cecc3c5

…roMq

introduce ZeroMqUtils, for common Zero MQ utilities functions

6e06a09

use ZeroMqUtils.bindSocket in ZeroMqMessageProducer

1604b3c

refactor ZeroMqMessageHandler providing connectUrl and bindPort sette…

89e7802

…rs and simple constructors, following the same logic used for ZeroMqMessageProvider

fix tests to follow the new ZeroMqMessageHandler implementation

71e6276

fix typo

7937f1c

add new updates to whats-new.adoc

d0c1e07

artembilan requested changes Jun 20, 2024

View reviewed changes

alessiomatricardi added 3 commits June 20, 2024 19:03

address ZeroMQUtils comments

f8ecae0

remove connectUrl and boundPort setters, add Javadoc to new constructors

7a1f7ee

add new DLS constructor for random port

bcac0a7

artembilan requested changes Jun 20, 2024

View reviewed changes

alessiomatricardi added 2 commits June 29, 2024 18:19

add since closure in ZeroMqUtils

1348389

add DSL support methods and specific that, when not defined, the sock…

920c127

…et will be bound to a random port

alessiomatricardi force-pushed the zeromq-handler-bind branch from 62934de to 920c127 Compare June 29, 2024 16:20

artembilan merged commit 272dde8 into spring-projects:main Jul 1, 2024
3 checks passed

GH-9228: ZeroMQ - provide binding to ZeroMqMessageHandler #9229

GH-9228: ZeroMQ - provide binding to ZeroMqMessageHandler #9229

Uh oh!

Conversation

alessiomatricardi commented Jun 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

artembilan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artembilan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alessiomatricardi commented Jun 17, 2024

Uh oh!

artembilan commented Jun 18, 2024

Uh oh!

artembilan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alessiomatricardi commented Jun 20, 2024

Uh oh!

artembilan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alessiomatricardi commented Jun 29, 2024

Uh oh!

Uh oh!

artembilan commented Jul 1, 2024

Uh oh!

Uh oh!

alessiomatricardi commented Jun 11, 2024 •

edited

Loading