-
Notifications
You must be signed in to change notification settings - Fork 219
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
Add MQTT message expiry interval #1756
Add MQTT message expiry interval #1756
Conversation
Signed-off-by: Dmitriy Barbul <dimabarbul@gmail.com>
Signed-off-by: Dmitriy Barbul <dimabarbul@gmail.com>
Signed-off-by: Dmitriy Barbul <dimabarbul@gmail.com>
Thanks a lot for the contribution @dimabarbul |
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.
Hey @dimabarbul
Looks really good, thanks a lot. 👍
I only added a few comments, mainly about missing javadoc on public methods.
...ity/model/src/main/java/org/eclipse/ditto/connectivity/model/mqtt/MessageExpiryInterval.java
Outdated
Show resolved
Hide resolved
...ity/model/src/main/java/org/eclipse/ditto/connectivity/model/mqtt/MessageExpiryInterval.java
Show resolved
Hide resolved
...ity/model/src/main/java/org/eclipse/ditto/connectivity/model/mqtt/MessageExpiryInterval.java
Show resolved
Hide resolved
...ity/model/src/main/java/org/eclipse/ditto/connectivity/model/mqtt/MessageExpiryInterval.java
Show resolved
Hide resolved
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.
LGTM, thanks for the contribution @dimabarbul 👍
Resolves #1729
Please, be patient. As I'm C# developer, I assume, there are moments done in non-Java way. If you find some names or language constructs that feel strange or non-Java-like, I'd be grateful if you point them out to me.
My concerns are:
In class
ExternalMessageToMqttPublishTransformerTest
constantRETAIN_FALL_BACK
was not used. It might have been used in method:I believe, as long as the method is named as "*FallsBackToFalse" it's ok to use false instead of class constant.
I'm not sure about implementation of class
MessageExpiryInterval
, especially about methodgetAsOptionalLong
. I was choosing betweenOptional<Long>
andOptionalLong
. I choseOptionalLong
because it doesn't do boxing and it has only 2 states - does or doesn't have value, whereasOptional<Long>
allows 3 states - doesn't have value, hasnull
value and has not-null
value.Header name
mqtt.message-expiry-interval
seems a bit long. I thought aboutmqtt.expiry
but decided to stick with MQTT 5 name.In class
GenericMqttPublish
I didn't know what to do when we got message from broker with invalid message expiry interval. I decided to not throw any exception and just fall back to no message expiry interval. Probably, need to log something here. Throwing exception doesn't seem to be an option here, cause it'll be needed to be added to signatures of whole call tree of this method.I added MQTT version to enum MqttHeader. Not sure if adding this kind of info to enum is OK in general.
I consider adding message expiry interval to last will message as out-of-scope.