search

vert-x3 / vertx-mqtt

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

MQTT5 server-side support #194

Closed paul-lysak closed 2 years ago

paul-lysak commented 3 years ago

MQTT5 server-side support for messages that already exist (i.e. AUTH message not included yet, client-side changes are out of scope). Part of https://github.com/vert-x3/vertx-mqtt/issues/161.

vietj commented 3 years ago

@paul-lysak is it ready for review ?

paul-lysak commented 3 years ago

@paul-lysak is it ready for review ?

Yes, for 2 month already

vietj commented 3 years ago

my bad I missed that

vietj commented 3 years ago

we will review this soon and make it part of 4.1.x (hopefully 4.1.1)

vietj commented 3 years ago

@ppatierno can you review this ?

vietj commented 3 years ago

a few initial remarks

paul-lysak commented 3 years ago

a few initial remarks

  • use of @GenIgnore(GenIgnore.PERMITTED_TYPE) instead of @GenIgnore which will provide usable API for RxJava and Munity
  • rename the new handlers with message to message handlers

Applied all the suggestions

paul-lysak commented 3 years ago

I have no idea how the changes can lead to this:

Results :

Tests in error: 
  Mqtt5ServerMaxMessageSizeTest.publishBigMessage:76 » Timeout Timed out
  MqttServerMaxMessageSizeTest.publishBigMessage » Timeout
  MqttServerWebSocketMaxMessageSizeTest.publishBigMessage » Timeout

Instable test server or test environment?

vietj commented 3 years ago

does it pass locally ?

On Thu, Jun 3, 2021 at 9:27 AM Paul Lysak @.***> wrote:

I have no idea how the changes can lead to this:

Results :

Tests in error:

Mqtt5ServerMaxMessageSizeTest.publishBigMessage:76 » Timeout Timed out

MqttServerMaxMessageSizeTest.publishBigMessage » Timeout

MqttServerWebSocketMaxMessageSizeTest.publishBigMessage » Timeout

Instable test server or test environment?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/vert-x3/vertx-mqtt/pull/194#issuecomment-853643794, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABXDCWO65ZPMLLCTGKCDTTTQ4VE3ANCNFSM4ZAKIMAA .

paul-lysak commented 3 years ago

does it pass locally ? … On Thu, Jun 3, 2021 at 9:27 AM Paul Lysak @.***> wrote: I have no idea how the changes can lead to this: Results : Tests in error: Mqtt5ServerMaxMessageSizeTest.publishBigMessage:76 » Timeout Timed out MqttServerMaxMessageSizeTest.publishBigMessage » Timeout MqttServerWebSocketMaxMessageSizeTest.publishBigMessage » Timeout Instable test server or test environment? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#194 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABXDCWO65ZPMLLCTGKCDTTTQ4VE3ANCNFSM4ZAKIMAA .

Yes, it does:

mvn -q clean verify -B
...
Results :

Tests run: 97, Failures: 0, Errors: 0, Skipped: 0
plysak@plysak-HP-ZBook-15u-G3 ~/develop/simplematter_forks/vertx-mqtt $ java --version
openjdk 13.0.4 2020-07-14
OpenJDK Runtime Environment (build 13.0.4+8-Ubuntu-120.04)
OpenJDK 64-Bit Server VM (build 13.0.4+8-Ubuntu-120.04, mixed mode)
plysak@plysak-HP-ZBook-15u-G3 ~/develop/simplematter_forks/vertx-mqtt $ mvn --version
Apache Maven 3.6.3
Maven home: /usr/share/maven
Java version: 13.0.4, vendor: Private Build, runtime: /usr/lib/jvm/java-13-openjdk-amd64
Default locale: it_IT, platform encoding: UTF-8
OS name: "linux", version: "5.4.0-53-generic", arch: "amd64", family: "unix"
plysak@plysak-HP-ZBook-15u-G3 ~/develop/simplematter_forks/vertx-mqtt $
vietj commented 3 years ago

ok, I will have a look

On Thu, Jun 3, 2021 at 9:41 AM Paul Lysak @.***> wrote:

does it pass locally ? … <#m3525398611991576673> On Thu, Jun 3, 2021 at 9:27 AM Paul Lysak @.***> wrote: I have no idea how the changes can lead to this: Results : Tests in error: Mqtt5ServerMaxMessageSizeTest.publishBigMessage:76 » Timeout Timed out MqttServerMaxMessageSizeTest.publishBigMessage » Timeout MqttServerWebSocketMaxMessageSizeTest.publishBigMessage » Timeout Instable test server or test environment? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#194 (comment) https://github.com/vert-x3/vertx-mqtt/pull/194#issuecomment-853643794>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABXDCWO65ZPMLLCTGKCDTTTQ4VE3ANCNFSM4ZAKIMAA .

Yes, it does:

mvn -q clean verify -B

...

Results :

Tests run: 97, Failures: 0, Errors: 0, Skipped: 0

@.***HP-ZBook-15u-G3 ~/develop/simplematter_forks/vertx-mqtt $ java --version

openjdk 13.0.4 2020-07-14

OpenJDK Runtime Environment (build 13.0.4+8-Ubuntu-120.04)

OpenJDK 64-Bit Server VM (build 13.0.4+8-Ubuntu-120.04, mixed mode)

@.***HP-ZBook-15u-G3 ~/develop/simplematter_forks/vertx-mqtt $ mvn --version

Apache Maven 3.6.3

Maven home: /usr/share/maven

Java version: 13.0.4, vendor: Private Build, runtime: /usr/lib/jvm/java-13-openjdk-amd64

Default locale: it_IT, platform encoding: UTF-8

OS name: "linux", version: "5.4.0-53-generic", arch: "amd64", family: "unix"

@.***HP-ZBook-15u-G3 ~/develop/simplematter_forks/vertx-mqtt $

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/vert-x3/vertx-mqtt/pull/194#issuecomment-853655082, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABXDCXW4SONP7OMFGAUFLDTQ4W4BANCNFSM4ZAKIMAA .

vietj commented 3 years ago

it is a general CI issue : https://github.com/vert-x3/vertx-mqtt/runs/2734205494?check_suite_focus=true

vietj commented 3 years ago

I will have a look at it soon.

paul-lysak commented 3 years ago

I will have a look at it soon.

ok, thanks

vietj commented 3 years ago

CI failure is due to a change in Netty with the max message size check

vietj commented 3 years ago

I'm investigating more

vietj commented 3 years ago

this commit introduces the issue https://github.com/netty/netty/commit/c32c520edde1bd554314f51d76b3eda4858e404c#diff-92a1a3695026063eb071c681ee3f6861f40a33a145c7e6e682798882fe46a494

vietj commented 3 years ago

I do have a PR fix for this issue and will submit to Netty in the meantime we will disable those tests and open an issue for it

vietj commented 3 years ago

@paul-lysak can you rebase on latest master ? failing tests have been commented until https://github.com/vert-x3/vertx-mqtt/issues/202 is resolved

paul-lysak commented 3 years ago

@paul-lysak can you rebase on latest master ? failing tests have been commented until #202 is resolved

done

paul-lysak commented 3 years ago

Seems one still remains:


Results :

Tests in error: 
  Mqtt5ServerMaxMessageSizeTest.publishBigMessage:76 » Timeout Timed out

Tests run: 98, Failures: 0, Errors: 1, Skipped: 2
vietj commented 3 years ago

I think it is a new test that you have in your PR as it mentions MQTT5 so you should @Ignore it in your PR

On Thu, Jun 3, 2021 at 4:23 PM Paul Lysak @.***> wrote:

Seems one still remains:

Results :

Tests in error:

Mqtt5ServerMaxMessageSizeTest.publishBigMessage:76 » Timeout Timed out

Tests run: 98, Failures: 0, Errors: 1, Skipped: 2

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/vert-x3/vertx-mqtt/pull/194#issuecomment-853909471, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABXDCXQOQ6CD6VG446HFBDTQ6F6XANCNFSM4ZAKIMAA .

paul-lysak commented 3 years ago

I think it is a new test that you have in your PR as it mentions MQTT5 so you should @ignore it in your PR … On Thu, Jun 3, 2021 at 4:23 PM Paul Lysak @.***> wrote: Seems one still remains: Results : Tests in error: Mqtt5ServerMaxMessageSizeTest.publishBigMessage:76 » Timeout Timed out Tests run: 98, Failures: 0, Errors: 1, Skipped: 2 — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#194 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABXDCXQOQ6CD6VG446HFBDTQ6F6XANCNFSM4ZAKIMAA .

Oh, right.. Sorry, disabled that test.

vietj commented 3 years ago

thanks CI is now green

vietj commented 3 years ago

we should use GenIgnore.PERMITTED_TYPE when @GenIgnore, I did not comment everywhere (but did many).

In the future (like Vertx 4.2) we will improve this and avoid usage of GenIgnore (i.e it would be the default).

paul-lysak commented 3 years ago

we should use GenIgnore.PERMITTED_TYPE when @GenIgnore, I did not comment everywhere (but did many).

In the future (like Vertx 4.2) we will improve this and avoid usage of GenIgnore (i.e it would be the default).

Ok, updated wherever I've found that.

vietj commented 3 years ago

a few comments have been made @paul-lysak

vietj commented 3 years ago

right this is a good point

On Tue, Jun 15, 2021 at 1:36 PM Paul Lysak @.***> wrote:

@paul-lysak commented on this pull request.

In src/main/java/io/vertx/mqtt/MqttEndpoint.java:

@@ -232,6 +272,16 @@ @Fluent MqttEndpoint publishReceivedHandler(Handler handler);

  • /**
    • Set the pubrec handler on the MQTT endpoint. This handler is called when a PUBREC
    • message is received by the remote MQTT client
  • *
    • @param handler the handler
    • @return a reference to this, so the API can be used fluently
  • */
  • @Fluent
  • MqttEndpoint publishReceivedMessageHandler(Handler handler);

What should we do with MqttEndpoint publishReceivedHandler(Handler handler); then?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

vietj commented 3 years ago

right this is a good point.

On Tue, Jun 15, 2021 at 1:36 PM Paul Lysak @.***> wrote:

@.**** commented on this pull request.

In src/main/java/io/vertx/mqtt/MqttEndpoint.java https://github.com/vert-x3/vertx-mqtt/pull/194#discussion_r651703191:

@@ -242,6 +292,17 @@ @Fluent MqttEndpoint publishReleaseHandler(Handler handler);

  • /**
    • Set the pubrel handler on the MQTT endpoint. This handler is called when a PUBREL
    • message is received by the remote MQTT client
  • *
    • @param handler the handler
    • @return a reference to this, so the API can be used fluently
  • */
  • @Fluent
  • MqttEndpoint publishReleaseMessageHandler(Handler handler);

What should we do with MqttEndpoint publishReleaseHandler(Handler handler); then?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/vert-x3/vertx-mqtt/pull/194#discussion_r651703191, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABXDCWA4EGSKBINJ7J4IVLTS43LTANCNFSM4ZAKIMAA .

vietj commented 3 years ago

right this is a good point.

On Tue, Jun 15, 2021 at 1:36 PM Paul Lysak @.***> wrote:

@.**** commented on this pull request.

In src/main/java/io/vertx/mqtt/MqttEndpoint.java https://github.com/vert-x3/vertx-mqtt/pull/194#discussion_r651703388:

@@ -252,6 +313,16 @@ @Fluent MqttEndpoint publishCompletionHandler(Handler handler);

  • /**
    • Set the pubcomp handler on the MQTT endpoint. This handler is called when a PUBCOMP
    • message is received by the remote MQTT client
  • *
    • @param handler the handler
    • @return a reference to this, so the API can be used fluently
  • */
  • @Fluent
  • MqttEndpoint publishCompletionMessageHandler(Handler handler);

What should we do with MqttEndpoint publishCompletionHandler(Handler handler); then?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/vert-x3/vertx-mqtt/pull/194#discussion_r651703388, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABXDCXSFRYDCDGSJ4ENUATTS43MZANCNFSM4ZAKIMAA .

vietj commented 3 years ago

@paul-lysak I think for now I will merge this branch in a specific MQTT V5 branch of the project as first milestone

there are several reasons for this

How does that sound to you ?

paul-lysak commented 3 years ago

@paul-lysak I think for now I will merge this branch in a specific MQTT V5 branch of the project as first milestone

there are several reasons for this

  • I would like this to be released in Vert.x 4.2 and for now master is 4.1
  • we are still missing documentation for this

  • I would like to bring extra improvements in this branch

    • flow control : reading V5 new features I can see that now flow control is provided and we should find a way to implement that in the server. Perhaps not in this PR as it would be an extra construct, i.e ReadStream / WriteStream of message that would allow to interact with the messages under flow control, e.g endpoint.publisher("my-topic") that would return a WriteStream<MqttPublishMessage>
    • have message interface to be mutable : currently it is immutable and I find that there is more overloading than necessary (e.g in publish), so we would make MqttPublishMessage mutable and have a publish method that uses a message to publish message

How does that sound to you ?

I can write some docs if you'd like.

As for the remaining features - I'm worried that using streams for publishing the QoS1 and QoS2 messages will move the control for QoS guarantees from application (which actually knows when the message is processed) into the internals of vertx-mqtt lib (which can only guess). That's, actually, the reason why we didn't use the client part of vertx-mqtt as it is - it gave too little control over the acknowledgement of the messages.

I don't like the idea of making the messages mutable, they represent something that application has asked to send to the counterpart. If vertx-mqtt needs to inform the application about some reaction - there are callbacks, futures, etc.

We've pretty much shared in this PR what we've done in our internal fork to fit our use cases. Besides the docs for this piece of work, it makes sense that someone else who needs to cover other use cases picks here - we just may not have an expertise of everyone's needs.

vietj commented 3 years ago

docs would be needed.

about flow control this would be an optional API to use.

On Tue, Jun 15, 2021 at 2:09 PM Paul Lysak @.***> wrote:

@paul-lysak https://github.com/paul-lysak I think for now I will merge this branch in a specific MQTT V5 branch of the project as first milestone

there are several reasons for this

-

I would like this to be released in Vert.x 4.2 and for now master is 4.1

we are still missing documentation for this

I would like to bring extra improvements in this branch

  • flow control : reading V5 new features I can see that now flow control is provided and we should find a way to implement that in the server. Perhaps not in this PR as it would be an extra construct, i.e ReadStream / WriteStream of message that would allow to interact with the messages under flow control, e.g endpoint.publisher("my-topic") that would return a WriteStream
    • have message interface to be mutable : currently it is immutable and I find that there is more overloading than necessary (e.g in publish), so we would make MqttPublishMessage mutable and have a publish method that uses a message to publish message

How does that sound to you ?

I can write some docs if you'd like.

As for the remaining features - I'm worried that using streams for publishing the QoS1 and QoS2 messages will move the control for QoS guarantees from application (which actually knows when the message is processed) into the internals of vertx-mqtt lib (which can only guess). That's, actually, the reason why we didn't use the client part of vertx-mqtt as it is - it gave too little control over the acknowledgement of the messages.

I don't like the idea of making the messages mutable, they represent something that application has asked to send to the counterpart. If vertx-mqtt needs to inform the application about some reaction - there are callbacks, futures, etc.

We've pretty much shared in this PR what we've done in our internal fork to fit our use cases. Besides the docs for this piece of work, it makes sense that someone else who needs to cover other use cases picks here - we just may not have an expertise of everyone's needs.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/vert-x3/vertx-mqtt/pull/194#issuecomment-861444097, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABXDCUYHLJBPGC7KLJ5XQTTS47H3ANCNFSM4ZAKIMAA .

vietj commented 3 years ago

so after you contribute docs I'll merge this in a feature branch and I'll add support for optional flow control

vietj commented 3 years ago

about making message mutable, the client/server would not mutate message, only the sender. The idea is to providea a builder style API over the current overloading, i.e endpoint.publish(MqttPublishMessage.create().topic("foo").qosLevel(MqttQoS.EXACTLY_ONCE)...)

vietj commented 3 years ago

actually I don't think we would make the message mutable for now

paul-lysak commented 3 years ago

@vietj updated the docs in src/main/asciidoc/index.adoc according to the MQTT v5 changes

vietj commented 3 years ago

thanks @paul-lysak can you open a new PR against https://github.com/vert-x3/vertx-mqtt/tree/mqtt-server-v5 ?

paul-lysak commented 3 years ago

thanks @paul-lysak can you open a new PR against https://github.com/vert-x3/vertx-mqtt/tree/mqtt-server-v5 ?

To check that I've got you right - would you like me to cancel this PR and open the new PR with the same changes but directed into another branch?

vietj commented 3 years ago

yes that's it :-) so we can keep track of all the work in this branch

On Wed, Jun 23, 2021 at 4:56 PM Paul Lysak @.***> wrote:

thanks @paul-lysak https://github.com/paul-lysak can you open a new PR against https://github.com/vert-x3/vertx-mqtt/tree/mqtt-server-v5 ?

To check that I've got you right - would you like me to cancel this PR and open the new PR with the same changes but directed into another branch?

— 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/194#issuecomment-866910709, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABXDCUWK37TGQBZT2FN7LDTUHYZ7ANCNFSM4ZAKIMAA .

paul-lysak commented 3 years ago

yes that's it :-) so we can keep track of all the work in this branch … On Wed, Jun 23, 2021 at 4:56 PM Paul Lysak @.***> wrote: thanks @paul-lysak https://github.com/paul-lysak can you open a new PR against https://github.com/vert-x3/vertx-mqtt/tree/mqtt-server-v5 ? To check that I've got you right - would you like me to cancel this PR and open the new PR with the same changes but directed into another branch? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#194 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABXDCUWK37TGQBZT2FN7LDTUHYZ7ANCNFSM4ZAKIMAA .

Here: https://github.com/vert-x3/vertx-mqtt/pull/204

vietj commented 3 years ago

merged manually in master now that it is versioned as 4.2 : https://github.com/vert-x3/vertx-mqtt/commit/e65bfb711773739a5e385cd07dd025e595aab711

thanks for the contribution, looking forward client support :-)

vietj commented 2 years ago

merged elsewhere

vietj commented 2 years ago

keeping this issue as reminder