Azure-Samples / MqttApplicationSamples

Samples implementing common PubSub patterns for Edge and Cloud Brokers
MIT License
26 stars 26 forks source link

ConnectionSettings should use "clean_start" instead of "clean_session" #83

Open cartertinney opened 1 year ago

cartertinney commented 1 year ago

"clean_session" is a term used from MQTT 3.1.1, "clean_start" would be the correct term for usage with MQTT 5 to prevent confusion

rido-min commented 1 year ago

yes this is a good point, before making any change I'd like to understand how the different MQTT libraries used in this repo behave when using MQTT5 and CleanSession:

https://docs.oasis-open.org/mqtt/mqtt/v5.0/os/mqtt-v5.0-os.html#_Toc3901039

I believe there is also some dependency with SessionExpiryInterval, without setting it I'm not able to get a persistent session with MQTTNet, see https://docs.oasis-open.org/mqtt/mqtt/v5.0/os/mqtt-v5.0-os.html#_Toc3901048

We might want to add SessionExpiryInterval as a new ConnectionSetting, with a default value that will be applied when CleanStart = False

cartertinney commented 1 year ago

In Python, the Paho library will raise an exception if you attempt to set a "clean_session" with MQTTv5, yes. And similarly, any attempt to set a "clean_start" (besides the default value) while using MQTTv3.1.1 will also result in an exception. This makes sense to me, given what you point out about the Session Expiry Interval property, as well as the change to when the option is provided (i.e. lifespan of the option) - it appears the semantics change between 3 and 5, it's not just a naming change, and thus it is even more important that we handle this change correctly and idiomatically.

I would reiterate, that I really think that we ought to be concerned with the MQTT5 spec, and the high level concepts rather than minutia of specific library implementations. Whether or not any particular library allows us to use a deprecated value/option with MQTT5 configuration should not be what informs our decisions.

rido-min commented 1 year ago

and I agree !!!

We could focus on MQTT5 only, and rename CleanSession to CleanStart.

cartertinney commented 1 year ago

Yes, I agree, this is what we should do.