vertx-mqtt
vertx-mqtt copied to clipboard
vert-x3
Add an MQTT client session, which allows to automatically re-connect.
Motivation:
Trying to fix: https://github.com/smallrye/smallrye-reactive-messaging/issues/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
This is a first draft for adding an MQTT client session, for clean session = true only.
@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.
I am sorry, but I completely disagree. Also, in https://github.com/smallrye/smallrye-reactive-messaging/issues/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.
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.
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.
@cescoffier @vietj Do we move on as agreed upon? Or do I just ditch the whole effort and look for alternatives elsewhere?
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.
@vietj please take a look at the discussion going on https://github.com/smallrye/smallrye-reactive-messaging/issues/1181, thanks!
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 https://github.com/vert-x3/vertx-mqtt/pull/197#discussion_r635356763:
+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 https://github.com/vert-x3/vertx-mqtt/pull/197#discussion_r635356763, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABXDCSFQVWSOJUHBWNDOG3TOPKZNANCNFSM45AIMREA .
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.
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.
@vietj @ctron instead of continuing to review ignoring me, can we continue the discussion on the issue please?
@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
@ctron based on this https://github.com/smallrye/smallrye-reactive-messaging/pull/1228 got merged, I guess that we can close this one?
@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.
We all know my opinion :-) I will double check with @vietj ... I do really think that you fixed the issue at the right level.
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 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.
@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).