-
-
Notifications
You must be signed in to change notification settings - Fork 61
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.
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
feat: better qos support #323
feat: better qos support #323
Conversation
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Pull Request Test Coverage Report for Build 4003324759Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a bunch of comments. Great start! 🚀
src/adapters/mqtt/index.ts
Outdated
@@ -48,6 +48,9 @@ class MqttAdapter extends Adapter { | |||
protocol: url.protocol.substr(0, url.protocol.length - 1), | |||
clientId: serverBinding && serverBinding.clientId, | |||
clean: serverBinding && serverBinding.cleanSession, | |||
properties: { | |||
sessionExpiryInterval: serverBinding && serverBinding.sessionExpiryInterval |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The MQTT bindings (mqtt
and mqtt5
) don't have a sessionExpiryInterval
property. We have to evaluate if that's something the MQTT5 binding should have, otherwise, we can't make a reference to it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Session expiry interval is specified in the MQTT spec. I think we should add it. It's required to have persistent sessions, and utilize QoS in MQTT.
src/adapters/mqtt/index.ts
Outdated
@@ -58,15 +61,19 @@ class MqttAdapter extends Adapter { | |||
username: userAndPasswordSecurityReq ? process.env.GLEE_USERNAME : undefined, | |||
password: userAndPasswordSecurityReq ? process.env.GLEE_PASSWORD : undefined, | |||
ca: X509SecurityReq ? certs : undefined, | |||
protocolVersion: 5, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do this, this adapter is always going to use MQTT 5. This must be a conditional value. Maybe based on the bindings. If the mqtt
bindings exist then it's version 3.x, if the mqtt5
binding exists then it's version 5. Or maybe a configuration in the glee.config.js
file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MQTT specifies 3
for version 3.1 and 4
for version 3.1.1. So for the mqtt
binding, which version will be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd go for version 4 given it's the latest for 3.x (that's really weird 😅)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok sure
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
src/adapters/mqtt/index.ts
Outdated
const serverBinding = mqttServerBinding | ||
const protocolVersion = mqtt5ServerBinding ? 5 : 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should get protocolVersion
from the server object instead:
const serverBinding = mqttServerBinding | |
const protocolVersion = mqtt5ServerBinding ? 5 : 4 | |
const protocolVersion = parseInt(this.AsyncAPIServer.protocolVersion() || 4) | |
const serverBinding = protocolVersion === 5 ? mqtt5ServerBinding : mqttServerBinding |
Just because the mqtt5
exists in the AsyncAPI document it doesn't mean it should be used.
src/lib/message.ts
Outdated
* Indicates successfully processed the message | ||
*/ | ||
notifySuccessfulProcessing() { | ||
this.emit('successfullyProcessed') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's follow the structure we're using everywhere else in the code base:
this.emit('successfullyProcessed') | |
this.emit('processing:successful') |
src/lib/message.ts
Outdated
* Indicates failure in processing the message | ||
*/ | ||
notifyFailedProcessing() { | ||
this.emit('failedProcessing') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.emit('failedProcessing') | |
this.emit('processing:failed') |
@sudoshreyansh you've got some conflicts. It looks good to me already but you need to solve them before we can merge. |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
@fmvilas I have fixed the conflicts. Can you please re-review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 LET'S GOOOO!
/rtm |
Congratulations on finishing your mentorship program, @sudoshreyansh 🎉 💰 😄 |
🎉 This PR is included in version 0.16.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
sessionExpiryInterval
property of the MQTT server binding.MQTT versions before 5.0 cannot be supported due to the limitations in the protocol and the MQTT.js client library.
Related issue(s)
See also #27