Skip to content

MQTT5 support for netty-codec-mqtt #10483

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
Merged
merged 1 commit into from
Aug 31, 2020
Merged

MQTT5 support for netty-codec-mqtt #10483

merged 1 commit into from
Aug 31, 2020

Conversation

paul-lysak
Copy link
Contributor

Motivation:

MQTT Specification version 5 was released over a year ago,
netty-codec-mqtt should be changed to support it.

Modifications:

Added more message and header types in io.netty.handler.codec.mqtt
package in netty-coded-mqtt subproject,
changed MqttEncoder and MqttDecoder to handle them properly,
updated examples in netty-example.
Based on https://github.com/moquette-io/netty-mqtt5-codec.

Result:

netty-coded-mqtt supports both MQTT5 and MQTT3 now.

@netty-bot
Copy link

Can one of the admins verify this patch?

@paul-lysak paul-lysak mentioned this pull request Aug 14, 2020
Open
@paul-lysak paul-lysak force-pushed the mqtt5 branch 2 times, most recently from fc5dd1b to 714d09e Compare August 15, 2020 07:03
hyperxpro
hyperxpro suggested changes Aug 16, 2020
View reviewed changes
Copy link
Contributor

@hyperxpro hyperxpro left a comment

Choose a reason for hiding this comment

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

Few changes and do check for API breakage.

normanmaurer
normanmaurer requested changes Aug 18, 2020
View reviewed changes
Copy link
Member

@normanmaurer normanmaurer left a comment

Choose a reason for hiding this comment

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

I will have a closer look soon. That said you will need to fix all the API breakage

@paul-lysak paul-lysak force-pushed the mqtt5 branch 3 times, most recently from a59e65c to 95eb759 Compare August 19, 2020 06:10
normanmaurer
normanmaurer requested changes Aug 19, 2020
View reviewed changes
Copy link
Member

@normanmaurer normanmaurer left a comment

Choose a reason for hiding this comment

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

next round of review

@paul-lysak paul-lysak force-pushed the mqtt5 branch 2 times, most recently from 7d90e00 to 4df4ec7 Compare August 19, 2020 11:41
@normanmaurer normanmaurer requested a review from chrisvest August 19, 2020 14:50
@normanmaurer
Copy link
Member

@chrisvest this may be a good one for you to check as well to get more familiar with the code base :)

normanmaurer
normanmaurer requested changes Aug 20, 2020
View reviewed changes
normanmaurer
normanmaurer requested changes Aug 24, 2020
View reviewed changes
chrisvest
chrisvest reviewed Aug 24, 2020
View reviewed changes
codec-mqtt/src/test/java/io/netty/handler/codec/mqtt/MqttCodecTest.java
@@ -295,8 +316,8 @@ public void testUnknownMessageType() throws Exception {
final MqttMessage decodedMessage = (MqttMessage) out.get(0);
assertTrue(decodedMessage.decoderResult().isFailure());
Throwable cause = decodedMessage.decoderResult().cause();
assertTrue(cause instanceof IllegalArgumentException);
assertEquals("unknown message type: 15", cause.getMessage());
assertTrue(cause instanceof DecoderException);
Copy link
Member

Choose a reason for hiding this comment

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

The new exception makes sense, but I wonder if it's an API change. I don't see a lot of docs around this, so maybe it's unspecified.

@paul-lysak paul-lysak force-pushed the mqtt5 branch 3 times, most recently from 6652e05 to f577a8d Compare August 25, 2020 08:59
@chrisvest
Copy link
Member

@netty-bot test this please

@paul-lysak
Copy link
Contributor Author

Rebased to the fresh 4.1.
@netty-bot test this, please

@paul-lysak
Copy link
Contributor Author

@chrisvest looks like I can't trigger the bot. Could you trigger the tests, please?

@chrisvest
Copy link
Member

@netty-bot test this please

chrisvest
chrisvest approved these changes Aug 26, 2020
View reviewed changes
Copy link
Member

@chrisvest chrisvest left a comment

Choose a reason for hiding this comment

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

The code looks good, but I don't know enough about the MQTT protocol to comment on the correctness of the implementation and how well it follows the spec.

johnou
johnou suggested changes Aug 26, 2020
View reviewed changes
@normanmaurer
Copy link
Member

@netty-bot test this please

@normanmaurer
Copy link
Member

@paul-lysak great work! Please just address comment of @johnou and than we are ready to go :)

@paul-lysak
MQTT5 support for netty-codec-mqtt
9af3e57 
Motivation:

 MQTT Specification version 5 was released over a year ago,
 netty-codec-mqtt should be changed to support it.

Modifications:

  Added more message and header types in `io.netty.handler.codec.mqtt`
  package in `netty-coded-mqtt` subproject,
  changed `MqttEncoder` and `MqttDecoder` to handle them properly,
  added attribute `NETTY_CODEC_MQTT_VERSION` to track protocol version

Result:

  `netty-coded-mqtt` supports both MQTT5 and MQTT3 now.
@normanmaurer
Copy link
Member

@netty-bot test this please

@franz1981 franz1981 mentioned this pull request Aug 27, 2020
Merged
@franz1981
Copy link
Contributor

franz1981 commented Aug 29, 2020 •
edited
Loading

I strongly suggest to look at the improvements I have made for #10509 will try to look at this if possible

@paul-lysak
Copy link
Contributor Author

I strongly suggest to look at the improvements I have made for #10509 will try to look at this if possible

That's a nice improvement. Better do it one by one, however, as doing the same thing in different PRs most likely will cause conflicts.

@normanmaurer normanmaurer merged commit be2cd68 into netty:4.1 Aug 31, 2020
@normanmaurer
Copy link
Member

@paul-lysak thanks a lot for all the hard work!

@paul-lysak
Copy link
Contributor Author

@paul-lysak thanks a lot for all the hard work!

Cool! Thank you and other reviewers for helping to keep up with Netty quality!

normanmaurer pushed a commit that referenced this pull request Aug 31, 2020
@paul-lysak @normanmaurer
MQTT5 support for netty-codec-mqtt (#10483)
fb4a578 
Motivation:

 MQTT Specification version 5 was released over a year ago,
 netty-codec-mqtt should be changed to support it.

Modifications:

  Added more message and header types in `io.netty.handler.codec.mqtt`
  package in `netty-coded-mqtt` subproject,
  changed `MqttEncoder` and `MqttDecoder` to handle them properly,
  added attribute `NETTY_CODEC_MQTT_VERSION` to track protocol version

Result:

  `netty-codec-mqtt` supports both MQTT5 and MQTT3 now.
@normanmaurer normanmaurer added this to the 4.1.52.Final milestone Sep 8, 2020
@chenzhiguo
Copy link

Great!!

This was referenced May 30, 2021
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@normanmaurer normanmaurer normanmaurer requested changes

@chrisvest chrisvest chrisvest approved these changes

+3 more reviewers

@kachayev kachayev kachayev left review comments

@hyperxpro hyperxpro hyperxpro requested changes

@johnou johnou johnou approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.1.52.Final
Development

Successfully merging this pull request may close these issues.
9 participants
@paul-lysak @netty-bot @normanmaurer @chrisvest @franz1981 @chenzhiguo @johnou @kachayev @hyperxpro