Add an MQTT client session, which allows to automatically re-connect. #197

ctron · 2021-05-17T11:26:06Z

Motivation:

Trying to fix: smallrye/smallrye-reactive-messaging#1181

Conformance:

Your commits should be signed and you should have signed the Eclipse Contributor Agreement as explained in https://github.com/eclipse/vert.x/blob/master/CONTRIBUTING.md
Please also make sure you adhere to the code style guidelines: https://github.com/vert-x3/wiki/wiki/Vert.x-code-style-guidelines

ctron · 2021-05-17T11:27:24Z

This is a first draft for adding an MQTT client session, for clean session = true only.

Signed-off-by: Jens Reimann <[email protected]>

cescoffier

Looks great!

cescoffier · 2021-05-18T06:08:20Z

src/main/java/io/vertx/mqtt/impl/MqttClientSessionImpl.java

+   * @return The duration to wait.
+   */
+  private Duration nextDelay() {
+    return Duration.ofSeconds(10);


Should this be configurable?

Yes it should :)

cescoffier · 2021-05-18T06:08:59Z

src/main/java/io/vertx/mqtt/impl/MqttClientSessionImpl.java

+   * @param reason The reason message.
+   */
+  private void closeConnection(final String reason) {
+    closeConnection(new IOException(reason));


might be better to use a VertxEception, as anyway the stack trace is useless.

Signed-off-by: Jens Reimann <[email protected]>

ppatierno · 2021-05-18T07:48:42Z

@ctron @cescoffier tbh I do really not agree to have this implementation into the MQTT client. It's not MQTT specification we are implementing here, where the concept of "session" is completely different. It shoud be fixed at smallrye level. Reconnection it's not a matter of the MQTT client itself but of the app using the client. The MQTT client has to implement the 3.1.1 specification and nothing more; any other concern is at application level.

ctron · 2021-05-18T07:55:20Z

I am sorry, but I completely disagree. Also, in smallrye/smallrye-reactive-messaging#1181 we already agreed that this should land in the client implementation of vertx.

The MQTT client session is an opinionated construct, and this lives in its own class, re-using the existing client.

People are asking for this functionality for a while. And instead of making people's live harder, we should make their lives easier, if was expect them to adopt our technologies.

ppatierno · 2021-05-18T07:57:53Z

My strong opinion is that an MQTT client should implement MQTT specification and nothing more. Any other aspects is out of the scope of the client. If people want something we can provide them examples or as I said implement it at smallrye level. Starting to mix not MQTT specification stuff into an MQTT client would drive to create a monster. Everyone can start to ask features that are not part of the spec.

ppatierno · 2021-05-18T08:00:37Z

People asking for this feature for a while and not doing that in their own applications mean people who don't know about MQTT protocol and maybe they are using the wrong protocol or in the wrong way. We should educate people and not putting everything just because they ask something. This client should be a pure 3.1.1 specification implementation. Stop.

ctron · 2021-05-18T08:02:52Z

@cescoffier @vietj Do we move on as agreed upon? Or do I just ditch the whole effort and look for alternatives elsewhere?

ppatierno · 2021-05-18T08:04:39Z

Or do I just ditch the whole effort and look for alternatives elsewhere?

What are the alternatives we are talking about? Why the feature cannot be into smallrye if you are using it? Why if you can't continue to use the MQTT client and adding the reconnect logic into your application. I really don't think that Eclipse Paho MQTT implementations put some stuff out of specification inside the client. The same doesn't happen for other protocols like AMQP 1.0 as well.

Signed-off-by: Jens Reimann <[email protected]>

vietj · 2021-05-19T14:29:00Z

src/main/java/io/vertx/mqtt/MqttClientSession.java

+/**
+ * An MQTT client session.
+ */
+public interface MqttClientSession {


this should be annotated with @VertxGen

I just tried that, and it fails with:

[ERROR] diagnostic: /home/jreimann/git/vertx-mqtt/src/main/java/io/vertx/mqtt/MqttClientSession.java:41: error: Could not generate model for io.vertx.mqtt.MqttClientSession: @VertxGen can only declare methods and not io.vertx.mqtt.MqttClientSession public interface MqttClientSession { ^

Any ideas what could cause that?

ppatierno · 2021-05-19T14:39:18Z

@vietj please take a look at the discussion going on smallrye/smallrye-reactive-messaging#1181, thanks!

Signed-off-by: Jens Reimann <[email protected]>

vietj · 2021-05-19T15:58:58Z

inner classes

…

On Wed, May 19, 2021 at 5:32 PM Jens Reimann ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/main/java/io/vertx/mqtt/MqttClientSession.java <#197 (comment)>: > +import java.util.Objects; +import java.util.StringJoiner; + +import io.netty.handler.codec.mqtt.MqttQoS; +import io.vertx.codegen.annotations.Fluent; +import io.vertx.core.Future; +import io.vertx.core.Handler; +import io.vertx.core.Vertx; +import io.vertx.core.buffer.Buffer; +import io.vertx.mqtt.impl.MqttClientSessionImpl; +import io.vertx.mqtt.messages.MqttPublishMessage; + +/** + * An MQTT client session. + */ +public interface MqttClientSession { I just tried that, and it fails with: [ERROR] diagnostic: /home/jreimann/git/vertx-mqtt/src/main/java/io/vertx/mqtt/MqttClientSession.java:41: error: Could not generate model for io.vertx.mqtt.MqttClientSession: @VertxGen can only declare methods and not io.vertx.mqtt.MqttClientSession public interface MqttClientSession { ^ Any ideas what could cause that? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#197 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABXDCSFQVWSOJUHBWNDOG3TOPKZNANCNFSM45AIMREA> .

This required to move out the inner classes and to create interfaces for the event classes too. Signed-off-by: Jens Reimann <[email protected]>

Signed-off-by: Jens Reimann <[email protected]>

ctron · 2021-05-19T16:14:09Z

Yes, that was the problem. Also I had to create interfaces for the even classes. I had to re-organize that bit. You might not like the way I did that. Just let me know how, and I well re-organize and re-name.

vietj · 2021-05-19T16:20:08Z

sure thing, I'll continue the review tomorrow

…

On Wed, May 19, 2021 at 6:14 PM Jens Reimann ***@***.***> wrote: Yes, that was the problem. Also I had to create interfaces for the even classes. I had to re-organize that bit. You might not like the way I did that. Just let me know how, and I well re-organize and re-name. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

ppatierno · 2021-05-19T17:27:44Z

@vietj @ctron instead of continuing to review ignoring me, can we continue the discussion on the issue please?

vietj · 2021-05-19T21:31:32Z

@ppatierno I did not spend much time on the review yet and did a first pass on the code, I did not spot your comments

…f the client This is based on the content of PR vert-x3/vertx-mqtt#197

This is based on the content of PR vert-x3/vertx-mqtt#197

…ession" This is based on the content of PR vert-x3/vertx-mqtt#197

This is based on the content of PR vert-x3/vertx-mqtt#197 Fixes smallrye#1181

ppatierno · 2021-05-31T13:14:22Z

@ctron based on this smallrye/smallrye-reactive-messaging#1228 got merged, I guess that we can close this one?

ctron · 2021-05-31T13:17:37Z

@ctron based on this smallrye/smallrye-reactive-messaging#1228 got merged, I guess that we can close this one?

If you really think this PR isn't an improvement to the vertx-mqtt client, then feel free to close it.

ppatierno · 2021-05-31T13:26:21Z

We all know my opinion :-) I will double check with @vietj ... I do really think that you fixed the issue at the right level.

cescoffier · 2021-05-31T13:31:04Z

I don't believe that the connector is at the right level. It's a temporary solution as we need to get things moving.
Typically, being in a connector avoid annoying not using Reactive Messaging to use the feature.

ctron · 2021-05-31T13:37:45Z

I don't believe that the connector is at the right level. It's a temporary solution as we need to get things moving.
Typically, being in a connector avoid annoying not using Reactive Messaging to use the feature.

I should have made myself clear. I fully agree with what @cescoffier said. I also think that having re-connect capabilities is definitely an improvement, like other MQTT clients have as well. Making this optional is fine, but it should come "out of the box" from an MQTT client.

Actively deciding against having the feature, is a failure in my opinion.

ppatierno · 2021-05-31T13:42:25Z

@ctron please provide me a concrete example of a client doing this but not just a reconnect as Paho does because I have already said it's ok with me. I am not ok of tracing all subscriptions in something called "session" on the client (even when "session" is a completely different thing in MQTT spec).

ctron force-pushed the feature/mqtt_client_session_1 branch 8 times, most recently from ec88f5d to d68fc58 Compare May 17, 2021 12:31

ctron added 4 commits May 17, 2021 14:35

Fix the CI issues

1d12436

Signed-off-by: Jens Reimann <[email protected]>

Add an MQTT client session, which allows to automatically re-connect.

683b982

Signed-off-by: Jens Reimann <[email protected]>

Rename and relocate example

2c2d1d6

Signed-off-by: Jens Reimann <[email protected]>

Add license header

f54f3bd

Signed-off-by: Jens Reimann <[email protected]>

ctron force-pushed the feature/mqtt_client_session_1 branch from d68fc58 to f54f3bd Compare May 17, 2021 12:36

ctron added 5 commits May 17, 2021 16:12

Add a few first tests

03277f1

Signed-off-by: Jens Reimann <[email protected]>

Add some more tests

006ae67

Signed-off-by: Jens Reimann <[email protected]>

It's not Rust, it's Java.

0799093

Signed-off-by: Jens Reimann <[email protected]>

Announce the subscription state before the session state

d3cffd2

Signed-off-by: Jens Reimann <[email protected]>

Use the new state for evaluating the actions

e5d04eb

Signed-off-by: Jens Reimann <[email protected]>

ctron marked this pull request as ready for review May 17, 2021 16:04

Add some documentation

6a9a19a

Signed-off-by: Jens Reimann <[email protected]>

cescoffier requested changes May 18, 2021

View reviewed changes

ctron added 2 commits May 18, 2021 09:06

Change exception

697101b

Signed-off-by: Jens Reimann <[email protected]>

Cleanup

2b704fd

Signed-off-by: Jens Reimann <[email protected]>

ctron added 3 commits May 18, 2021 14:21

Allow using cleanSession=false

0b30383

Signed-off-by: Jens Reimann <[email protected]>

Patch through additional "publish" handlers

85a6c94

Signed-off-by: Jens Reimann <[email protected]>

Rerun codegen

9d3729d

Signed-off-by: Jens Reimann <[email protected]>

vietj reviewed May 19, 2021

View reviewed changes

Add annotation

b0cbff9

Signed-off-by: Jens Reimann <[email protected]>

ctron added 2 commits May 19, 2021 18:11

Annotate MqttClientSession with @VertxGen

e9d6ae1

This required to move out the inner classes and to create interfaces for the event classes too. Signed-off-by: Jens Reimann <[email protected]>

Apply formatter

2f83eca

Signed-off-by: Jens Reimann <[email protected]>

ctron added a commit to ctron/smallrye-reactive-messaging that referenced this pull request May 27, 2021

Fix broken MQTT client by implementing a reconnect "session" on top o…

b1d89ee

…f the client This is based on the content of PR vert-x3/vertx-mqtt#197

ctron added a commit to ctron/smallrye-reactive-messaging that referenced this pull request May 27, 2021

Fix broken MQTT client by implementing a reconnect "session"

c9c8b24

This is based on the content of PR vert-x3/vertx-mqtt#197

ctron added a commit to ctron/smallrye-reactive-messaging that referenced this pull request May 27, 2021

Fix broken MQTT client by implementing a reconnect "session"

d96aa2f

This is based on the content of PR vert-x3/vertx-mqtt#197

ctron mentioned this pull request May 27, 2021

Fix broken MQTT client by implementing a reconnect "session" smallrye/smallrye-reactive-messaging#1228

Merged

ctron added a commit to ctron/smallrye-reactive-messaging that referenced this pull request May 27, 2021

Fix broken MQTT client by implementing a reconnect "session"

3879c5a

This is based on the content of PR vert-x3/vertx-mqtt#197

ctron added a commit to ctron/smallrye-reactive-messaging that referenced this pull request May 27, 2021

[smallrye#1181] Fix broken MQTT client by implementing a reconnect "s…

566bc7d

…ession" This is based on the content of PR vert-x3/vertx-mqtt#197

ctron added a commit to ctron/smallrye-reactive-messaging that referenced this pull request May 27, 2021

Fix broken MQTT client by implementing a reconnect "session"

0235700

This is based on the content of PR vert-x3/vertx-mqtt#197 Fixes smallrye#1181

ctron added a commit to ctron/smallrye-reactive-messaging that referenced this pull request May 27, 2021

Fix broken MQTT client by implementing a reconnect "session"

2a7a382

This is based on the content of PR vert-x3/vertx-mqtt#197 Fixes smallrye#1181

ctron added a commit to ctron/smallrye-reactive-messaging that referenced this pull request May 28, 2021

Fix broken MQTT client by implementing a reconnect "session"

a920eca

This is based on the content of PR vert-x3/vertx-mqtt#197 Fixes smallrye#1181

ctron added a commit to ctron/smallrye-reactive-messaging that referenced this pull request May 28, 2021

Fix broken MQTT client by implementing a reconnect "session"

bda190e

This is based on the content of PR vert-x3/vertx-mqtt#197 Fixes smallrye#1181

ctron added a commit to ctron/smallrye-reactive-messaging that referenced this pull request May 31, 2021

Fix broken MQTT client by implementing a reconnect "session"

cdbeee1

This is based on the content of PR vert-x3/vertx-mqtt#197 Fixes smallrye#1181

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an MQTT client session, which allows to automatically re-connect. #197

Add an MQTT client session, which allows to automatically re-connect. #197

ctron commented May 17, 2021 •

edited

Loading

ctron commented May 17, 2021

cescoffier left a comment

cescoffier May 18, 2021

ctron May 18, 2021

cescoffier May 18, 2021

ctron May 18, 2021

ppatierno commented May 18, 2021

ctron commented May 18, 2021

ppatierno commented May 18, 2021

ppatierno commented May 18, 2021

ctron commented May 18, 2021

ppatierno commented May 18, 2021

vietj May 19, 2021

ctron May 19, 2021

ppatierno commented May 19, 2021

vietj commented May 19, 2021 via email

ctron commented May 19, 2021

vietj commented May 19, 2021 via email

ppatierno commented May 19, 2021

vietj commented May 19, 2021

ppatierno commented May 31, 2021

ctron commented May 31, 2021

ppatierno commented May 31, 2021

cescoffier commented May 31, 2021

ctron commented May 31, 2021

ppatierno commented May 31, 2021

Add an MQTT client session, which allows to automatically re-connect. #197

Are you sure you want to change the base?

Add an MQTT client session, which allows to automatically re-connect. #197

Conversation

ctron commented May 17, 2021 • edited Loading

ctron commented May 17, 2021

cescoffier left a comment

Choose a reason for hiding this comment

cescoffier May 18, 2021

Choose a reason for hiding this comment

ctron May 18, 2021

Choose a reason for hiding this comment

cescoffier May 18, 2021

Choose a reason for hiding this comment

ctron May 18, 2021

Choose a reason for hiding this comment

ppatierno commented May 18, 2021

ctron commented May 18, 2021

ppatierno commented May 18, 2021

ppatierno commented May 18, 2021

ctron commented May 18, 2021

ppatierno commented May 18, 2021

vietj May 19, 2021

Choose a reason for hiding this comment

ctron May 19, 2021

Choose a reason for hiding this comment

ppatierno commented May 19, 2021

vietj commented May 19, 2021 via email

ctron commented May 19, 2021

vietj commented May 19, 2021 via email

ppatierno commented May 19, 2021

vietj commented May 19, 2021

ppatierno commented May 31, 2021

ctron commented May 31, 2021

ppatierno commented May 31, 2021

cescoffier commented May 31, 2021

ctron commented May 31, 2021

ppatierno commented May 31, 2021

ctron commented May 17, 2021 •

edited

Loading