Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ZeroMQ: optional topic wrap #9197

Merged
merged 14 commits into from
Jun 10, 2024
Merged

ZeroMQ: optional topic wrap #9197

merged 14 commits into from
Jun 10, 2024

Conversation

alessiomatricardi
Copy link
Contributor

When a topic is specified to the the ZeroMqMessageHandler, it sends a message with the following format
Frame 1: byte array of the topic
Frame 2: empty byte array
Frame 3: byte array representing the message payload

Is there a reason over this choice?
The official ZeroMQ documentation doesn't offer a clear view of the better approach which has to be followed, however it provides some code examples of how send/receive ZeroMQ packets.
Here an example (taken from here)

//  Send a message on the 'status' topic
zmq_send (pub, "status", 6, ZMQ_SNDMORE);
zmq_send (pub, "All is well", 11, 0);

This example could suggest that no empty frames are needed, but choseable.

My PR addresses this issue (yes, for me this is a bug) keeping the default behaviour (wrap the topic with an additional empty ZeroMQ frame)

@pivotal-cla
Copy link

@alessiomatricardi Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@alessiomatricardi alessiomatricardi changed the title ZeroMQ: Make optionally topic wrap ZeroMQ: optional topic wrap May 31, 2024
@pivotal-cla
Copy link

@alessiomatricardi Thank you for signing the Contributor License Agreement!

artembilan
artembilan requested changes May 31, 2024
View reviewed changes
Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for contribution!
Please, consider my review.
Plus it would be great to have some tests around these new options on the adapters.
And add your name to the @author list of all the affected classes.

...romq/src/main/java/org/springframework/integration/zeromq/outbound/ZeroMqMessageHandler.java Outdated Show resolved Hide resolved
...romq/src/main/java/org/springframework/integration/zeromq/outbound/ZeroMqMessageHandler.java Outdated Show resolved Hide resolved
...romq/src/main/java/org/springframework/integration/zeromq/outbound/ZeroMqMessageHandler.java Outdated Show resolved Hide resolved
@alessiomatricardi
Copy link
Contributor Author

Done! Let me know if now it's good

artembilan
artembilan requested changes Jun 6, 2024
View reviewed changes
Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, please, take a look into zeromq.adoc to mention these two new properties in respective places when we explain those channel adapter.

Thank you!

...eromq/src/main/java/org/springframework/integration/zeromq/dsl/ZeroMqMessageHandlerSpec.java Outdated Show resolved Hide resolved
...romq/src/main/java/org/springframework/integration/zeromq/dsl/ZeroMqMessageProducerSpec.java Outdated Show resolved Hide resolved
...romq/src/main/java/org/springframework/integration/zeromq/inbound/ZeroMqMessageProducer.java Outdated Show resolved Hide resolved
...romq/src/main/java/org/springframework/integration/zeromq/inbound/ZeroMqMessageProducer.java Outdated Show resolved Hide resolved
...romq/src/main/java/org/springframework/integration/zeromq/outbound/ZeroMqMessageHandler.java Outdated Show resolved Hide resolved
...romq/src/main/java/org/springframework/integration/zeromq/outbound/ZeroMqMessageHandler.java Outdated Show resolved Hide resolved
artembilan
artembilan requested changes Jun 6, 2024
View reviewed changes
Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one minor suggestion.
And, please, take a look into docs.

...romq/src/main/java/org/springframework/integration/zeromq/inbound/ZeroMqMessageProducer.java Outdated Show resolved Hide resolved
@alessiomatricardi
Copy link
Contributor Author

And, please, take a look into docs.

Was just writing them :)

artembilan
artembilan requested changes Jun 6, 2024
View reviewed changes
Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something is off with tests:

 ZeroMqMessageProducerTests > testMessageProducerForPubSubDisabledWrapTopic() FAILED
    reactor.core.Exceptions$ReactorRejectedExecutionException at ZeroMqMessageProducerTests.java:188
        Caused by: java.util.concurrent.RejectedExecutionException at ZeroMqMessageProducerTests.java:188

ZeroMqMessageProducerTests > testMessageProducerForPair() FAILED
    reactor.core.Exceptions$ReactorRejectedExecutionException at ZeroMqMessageProducerTests.java:92
        Caused by: java.util.concurrent.RejectedExecutionException at ZeroMqMessageProducerTests.java:92

ZeroMqMessageProducerTests > testMessageProducerForPubSubReceiveRaw() FAILED
    org.zeromq.ZMQException at ZeroMqMessageProducerTests.java:100

Please, run locally ./gradlew :spring-integration-zeromq:check

artembilan
artembilan approved these changes Jun 6, 2024
View reviewed changes
Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll merge this when you fix that my last comment in doc.

@artembilan artembilan added this to the 6.4.0-M1 milestone Jun 6, 2024
artembilan
artembilan requested changes Jun 6, 2024
View reviewed changes
Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind looking close to those tests:

ZeroMqMessageProducerTests > testMessageProducerForPubSubDisabledWrapTopic() FAILED
    reactor.core.Exceptions$ReactorRejectedExecutionException at ZeroMqMessageProducerTests.java:188
        Caused by: java.util.concurrent.RejectedExecutionException at ZeroMqMessageProducerTests.java:188

ZeroMqMessageProducerTests > testMessageProducerForPubSubReceiveRaw() FAILED
    org.zeromq.ZMQException at ZeroMqMessageProducerTests.java:100

?

Looks like they are failing sporadically.
Perhaps they share some destinations or consumers. Or whatever, but as you see the build result is not stable.

Thanks

@artembilan
Copy link
Member

Still have one failing:

ZeroMqMessageProducerTests > testMessageProducerForPubSubDisabledWrapTopic() FAILED
    reactor.core.Exceptions$ReactorRejectedExecutionException at ZeroMqMessageProducerTests.java:188
        Caused by: java.util.concurrent.RejectedExecutionException at ZeroMqMessageProducerTests.java:188

@artembilan
Copy link
Member

No. Still doesn't go through:

 ZeroMqMessageProducerTests > testMessageProducerForPubSubDisabledWrapTopic() FAILED
    reactor.core.Exceptions$ReactorRejectedExecutionException at ZeroMqMessageProducerTests.java:184
        Caused by: java.util.concurrent.RejectedExecutionException at ZeroMqMessageProducerTests.java:184

ZeroMqMessageProducerTests > testMessageProducerForPair() FAILED
    reactor.core.Exceptions$ReactorRejectedExecutionException at ZeroMqMessageProducerTests.java:93
        Caused by: java.util.concurrent.RejectedExecutionException at ZeroMqMessageProducerTests.java:93

ZeroMqMessageProducerTests > testMessageProducerForPubSubReceiveRaw() FAILED
    reactor.core.Exceptions$ReactorRejectedExecutionException at ZeroMqMessageProducerTests.java:147
        Caused by: java.util.concurrent.RejectedExecutionException at ZeroMqMessageProducerTests.java:147

Sorry for being so annoying, but something is off with these new options or new tests.

@alessiomatricardi
Copy link
Contributor Author

Don't worry! Seems that something strange occurs during destroying, I will look into it. It's strange because they work well on my side when I call ./gradlew :spring-integration-zeromq:check.

@artembilan artembilan merged commit 80c8a61 into spring-projects:main Jun 10, 2024
3 checks passed
@artembilan
Copy link
Member

@alessiomatricardi ,

thank you for contribution; looking forward for more!

spring-builds pushed a commit that referenced this pull request Jun 10, 2024
@alessiomatricardi @spring-builds
GH-9197: Optional ZeroMQ topic wrapping
13370e4 
Fixes: #9197

* Update `ZeroMqMessageHandler` for `wrapTopic` option
* Add author, fix code style, add same logic also for `ZeroMqMessageProducer`
* Add `wrapTopic` function also in DSL specs
* Fix wrap topic test: duplicate socket address caused binding exception
* Rewrite the `MessageProducer.wrapTopic()` test
* Call `stop()` instead of `destroy()` method

(cherry picked from commit 80c8a61)
spring-builds pushed a commit that referenced this pull request Jun 10, 2024
@alessiomatricardi @spring-builds
GH-9197: Optional ZeroMQ topic wrapping
ac29b26 
Fixes: #9197

* Update `ZeroMqMessageHandler` for `wrapTopic` option
* Add author, fix code style, add same logic also for `ZeroMqMessageProducer`
* Add `wrapTopic` function also in DSL specs
* Fix wrap topic test: duplicate socket address caused binding exception
* Rewrite the `MessageProducer.wrapTopic()` test
* Call `stop()` instead of `destroy()` method

(cherry picked from commit 80c8a61)
This was referenced Jun 10, 2024
Closed
Closed
@alessiomatricardi alessiomatricardi deleted the patch-1 branch June 10, 2024 18:47
EddieChoCho pushed a commit to EddieChoCho/spring-integration that referenced this pull request Jun 26, 2024
@alessiomatricardi @EddieChoCho
spring-projectsGH-9197: Optional ZeroMQ topic wrapping
2ad4445 
Fixes: spring-projects#9197

* Update `ZeroMqMessageHandler` for `wrapTopic` option
* Add author, fix code style, add same logic also for `ZeroMqMessageProducer`
* Add `wrapTopic` function also in DSL specs
* Fix wrap topic test: duplicate socket address caused binding exception
* Rewrite the `MessageProducer.wrapTopic()` test
* Call `stop()` instead of `destroy()` method

**Auto-cherry-pick to `6.3.x` & `6.2.x`**
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@artembilan artembilan artembilan requested changes

Assignees
No one assigned
Labels
(EOL) for: backport-to-6.2.x for: backport-to-6.3.x in: zeromq status: backported type: enhancement
Projects
None yet
Milestone
6.4.0-M1
Development

Successfully merging this pull request may close these issues.
4 participants
@alessiomatricardi @pivotal-cla @artembilan @spring-builds