search

vert-x3 / vertx-mqtt

Vert.x MQTT
Apache License 2.0
184 stars 87 forks source link

MQTT v5 support #161

Open paul-lysak opened 4 years ago

paul-lysak commented 4 years ago

Hello. We'd like vertx-mqtt to support MQTT v5 (https://docs.oasis-open.org/mqtt/mqtt/v5.0/mqtt-v5.0.html) and we're willing to develop such changes. Before we started, could you give us some guidelines on how to make sure that our MQTT5-related changes are suitable for merging back into the upstream vertx-mqtt?

In particular, what's the best way to change netty-codec-mqtt - Netty maintainers don't seem to be interested in its further development (https://github.com/netty/netty/issues/6285), it makes no sense to expect MQTT5 support from them. So, if vertx-mqtt is to support MQTT v5 then there are 2 possible solutions:

What's your opinion on this?

ppatierno commented 4 years ago

Tbh I didn't get what the comment "This was moved to an alternative project" means? Which alternative project? I think that the matt v5 coded should be part of netty and not of this project as we do for mqtt 3.1. Why can't you work on mqtt 5 codec for netty and then reusing it here?

ppatierno commented 4 years ago

I found the codec was moved here https://github.com/moquette-io/netty-mqtt5-codec Maybe you can use it?

ppatierno commented 4 years ago

The only thing I see is about how much it's supported. There are just 2 commits and 2 years ago. :-(

paul-lysak commented 4 years ago

The only thing I see is about how much it's supported. There are just 2 commits and 2 years ago. :-(

That's the issue. Another problem is that its artifacts aren't deployed to any public Maven repository. I'll try to ping Netty maintainers again to see what they think on MQTT5. If they still don't respond - we can release an artifact based on https://github.com/moquette-io/netty-mqtt5-codec and do the changes in vertx-mqtt to use it.

paul-lysak commented 4 years ago

I've submitted a PR to the Netty: https://github.com/netty/netty/pull/10483 . It's based on https://github.com/moquette-io/netty-mqtt5-codec with some corrections and improvements. Let's see how will they react now :)

ppatierno commented 3 years ago

@paul-lysak and it was approved! Congrats! Now if you are willing to work on this for adding the support to the Vert.x MQTT library, I would say to:

  1. trying to do it in steps, not a huge PR but little PRs adding incremental features. It's easy to review for us.
  2. I would do only for the master, so for future Vert.x 4 and not for 3.9. I would expect too much work for backporting due to the differences. @vietj what's your opinion on this?

The first thing to do should be waiting for a new Netty release with the MQTT 5 support, bumping the version here and checking that nothing is broken with 3.1.1 :-)

paul-lysak commented 3 years ago

@paul-lysak and it was approved! Congrats! Now if you are willing to work on this for adding the support to the Vert.x MQTT library, I would say to:

  1. trying to do it in steps, not a huge PR but little PRs adding incremental features. It's easy to review for us.
  2. I would do only for the master, so for future Vert.x 4 and not for 3.9. I would expect too much work for backporting due to the differences. @vietj what's your opinion on this?

The first thing to do should be waiting for a new Netty release with the MQTT 5 support, bumping the version here and checking that nothing is broken with 3.1.1 :-)

Thank you! Will start work on it after the nearest Netty release. I'll try to split it into parts wherever possible.

paul-lysak commented 3 years ago

We've decided not to go forward with providing PR for this issue: I've tried to do the necessary changes and turned out that satisfying code generator (for classes marked with @VertxGen) will take more effort and require more wrapping than just copying and adapting to our use case. For example, the code generator was not happy when I've added to MqttConnAckMessage new create method that takes MqttProperties.

For those who decide to go forward - here's my assessment of the required changes:

vietj commented 3 years ago

@paul-lysak can you reconsider this choice if we help you on this ?

e.g if you annotate the method with @GenIgnore() then the code generator will ignore this method , etc...

paul-lysak commented 3 years ago

@vietj what is exact purpose of @VertxGen in MqttConnAckMessage and other classes? How do I know which methods I may ignore and which not? Tried to ignore original connect method - mvn verify seems to pass just fine. I may consider contributions, but it's not going to be a priority.

vietj commented 3 years ago

the goal is to code generate API in other languages, e.g the Vert.x RxJava 2 API is generated using it.

in general just add @GenIgnore everywhere you need it in VertxGen class and if we can share a working branch we (maintainer of vertx) will take care of making this work for you.

paul-lysak commented 3 years ago

Ok, reopened https://github.com/vert-x3/vertx-dependencies/pull/49 and https://github.com/vert-x3/vertx-mqtt/pull/168. Will try to contribute more occasionally.

manju-reddys commented 3 years ago

Mqtt V5 support will be added as part of Vertx V4 release?

vietj commented 3 years ago

@manju-reddys we are missing contributors for this, so it will be added if somebody contributes it

vietj commented 3 years ago

@paul-lysak is motivated to contribute it, I don't know what's the current status of his contribution

paul-lysak commented 3 years ago

@vietj all the contributions are blocked by https://github.com/vert-x3/vertx-dependencies/pull/49

vietj commented 3 years ago

@paul-lysak yes I remember now.

I think you can try override these dependencies in the vertx-mqtt pom file of a working branch until we sort this out ?

paul-lysak commented 3 years ago

I've started to work on back-porting MQTT5 changes from our system to this repository. @vietj you can see the work in progress here: https://github.com/simplematter/vertx-mqtt/tree/mqtt5 . Let me know if you have any feedback on that.

vietj commented 3 years ago

thanks for heads up @paul-lysak !

vietj commented 3 years ago

@paul-lysak FYI Netty was bumped to Netty 4.1.60.Final in master

paul-lysak commented 3 years ago

Here comes server-side support (except of AUTH message): https://github.com/vert-x3/vertx-mqtt/pull/194

chenzhiguo commented 3 years ago

Has this feature been developed yet? Do you need any more help?

joggeli34 commented 2 years ago

Are there any plans, that the client also supports v5?

vietj commented 2 years ago

any contribution would be accepted, the core team has no bandwidth for this currently

On Sat, Feb 5, 2022 at 8:26 AM joggeli34 @.***> wrote:

Are there any plans, that the client also supports v5?

— Reply to this email directly, view it on GitHub https://github.com/vert-x3/vertx-mqtt/issues/161#issuecomment-1030568877, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABXDCV2BCZP7QAPEUI5KRTUZTGJTANCNFSM4OZQMXLA . You are receiving this because you were mentioned.Message ID: @.***>

mattisonchao commented 1 year ago

@vietj I will work on it.

vietj commented 1 year ago

looking forward for this @mattisonchao

qianwj commented 2 months ago

@vietj hi, I wrote the last part of mqtt version 5. Would you have any time to review it? https://github.com/vert-x3/vertx-mqtt/pull/248

vietj commented 2 months ago

thanks @qianwj will do this asap

ppatierno commented 2 months ago

@qianwj thanks! I will do the same asap