moleculerjs / moleculer

:rocket: Progressive microservices framework for Node.js
https://moleculer.services/
MIT License
6.15k stars 583 forks source link

Mqtt Transporter QoS #306

Closed chrvadala closed 6 years ago

chrvadala commented 6 years ago

Is your feature request related to a problem? Please describe. Hi @icebob, I was thinking about the Quality of Service of the MQTT transporter (rel: https://github.com/mqttjs/MQTT.js#about-qos) Actually the MQTT Transporter uses QoS 0 to deliver messages and there isn't a way to configure it. Is there a specific reason or is it just a missing feature? Have you thought about pros and cons? I think that could be better QoS 1, because actually the callback is called without consider if the broker received the message. QoS 1 introduces a server ACK and the callback contains the result of this.

LMK your thought. I may work on a PR that introduces a transporter option to configure the QoS. About the default I think that for now we can keep QoS 0, to avoid compatibility break and change it in the future with QoS 1. (rel: https://github.com/mqttjs/MQTT.js#publish) (rel: https://github.com/mqttjs/MQTT.js#mqttclientsubscribetopictopic-arraytopic-object-options-callback)

icebob commented 6 years ago

Hi, I'm not familiar with MQTT QoS, so I can't say it will work properly with system messages like INFO, DISCOVER, DISCONNECT...etc. But if you can create a PR we will see it.

For the PR: I suggest follow the Kafka options, so please create a publish: {} options which will be passed entirely to the mqtt publish as 3rd params. Similar to it, make a subscribe:{} an pass to subscribe because I see it has a same qos option.

chrvadala commented 6 years ago

Great! I will work on it ;)

icebob commented 6 years ago

Any news? Or I can close it?

chrvadala commented 6 years ago

Hi Icebob, recently I haven't had enough time to work on it, but a month ago I wrote this feature. I haven't send a PR yet, because I'd like to test it on an environment, but consider that the unit tests are passing.

Let me show you the idea. I introduced a new option that is a boolean qosZero. I think that the default should be false because the only QoS that can guarantee that a message is received across services is 1.

The callback that your are using to resolve the Promise is (by default) called on nextTick. If you want to be sure that the message has been sent you need QoS >= 1

From https://github.com/mqttjs/MQTT.js#mqttclientpublishtopic-message-options-callback callback - function (err), fired when the QoS handling completes, or at the next tick if QoS 0. An error occurs if client is disconnecting.

For example I found that the DISCONNECT message is sometimes queued but not forwarded to the broker, because the Promise is resolved too early.

If you want we may release this feature as experimental feature with a default set to qosZero=true and add a warning like you do for TCP transport. This avoids breaking changes.

The code is available on this fork https://github.com/chrvadala/moleculer/tree/mqtt-qos

Some refs: http://www.steves-internet-guide.com/understanding-mqtt-qos-levels-part-1/ http://www.steves-internet-guide.com/understanding-mqtt-qos-2/

chrvadala commented 6 years ago

PR merged here https://github.com/moleculerjs/moleculer/pull/340 We can close this

icebob commented 6 years ago

Before the next breaking version (0.14) we should discuss the QoS default value. It would also be good to measure the performance between QoS 0 & 1.