[mqtt] Changed MQTT client from Paho to HiveMQ #1125

J-N-K · 2019-10-13T10:24:30Z

Supersedes #929.

Signed-off-by: Jan N. Klug jan.n.klug@rub.de

J-N-K · 2019-10-13T10:25:49Z

Points to discuss:

Should we define a netty core-feature? We have a lot of bundles that use netty and - at the moment - all can use 4.1.34.Final.
How shall we select Mqtt3/5 client? This requires a reconnection in any case, but setter/getter or constructor enhancement? We could also use Mqtt3 for the existing constructor and define a new one which allows the selevtion of v3/v5

wborn · 2019-10-13T11:17:58Z

Thanks for pushing this forwards. 👍

I think adding a netty feature is a good addition. Personally I would go for an additional constructor if the only configuration option is v3/v5. To future proof choosing between many options using a builder might be another option. But it's probably overkill if there's just this one configuration option.

davidgraeff

Thank you very much for this PR.

I've added a few inline comments.

...ansport.mqtt/src/main/java/org/eclipse/smarthome/io/transport/mqtt/MqttBrokerConnection.java

davidgraeff · 2019-10-20T10:04:33Z

...ansport.mqtt/src/main/java/org/eclipse/smarthome/io/transport/mqtt/MqttBrokerConnection.java

+
+        // use defaults when not provided by caller
+        qos = qos == null ? this.qos : qos;
+        retain = retain == null ? this.retain : retain;


You are using deprecated class fields in this non-deprecated method.

No way to remin backward compatible otherwise. When we remove the retain-setter/getter, we can default that to false (what is the default now, if not explicitely set otherwise).

davidgraeff · 2019-10-20T10:05:23Z

...ansport.mqtt/src/main/java/org/eclipse/smarthome/io/transport/mqtt/MqttBrokerConnection.java

@@ -890,7 +888,7 @@ public void publish(String topic, byte[] payload, MqttActionCallback listener) {
     *         exceptionally on an error or with a result of false if no broker connection is established.
     */
    public CompletableFuture<Boolean> publish(String topic, byte[] payload) {


Mark this deprecated maybe? Implicitly it uses deprecated class fields ("retain", "qos").

It'll use the default (QoS 0 and retain false) after the getter/setter is removed.

While we are discussing deprecation: What is the use-case for subscribeRaw and unsubscribeRaw? If there are no subscribers, why should the connection subscribe to a topic?

I think it had to do keep alive subscriptions. But not sure anymore, I should have documented those methods better. Did you encounter any usages?

Not in openhab2-addons and not in openhab-core

Deprecating or better removing should be fine then. I also do not see any benefit in keeping this API.

davidgraeff · 2019-10-20T10:09:16Z

...ansport.mqtt/src/main/java/org/eclipse/smarthome/io/transport/mqtt/MqttBrokerConnection.java

-        }
-        return f;
+        CompletableFuture<Boolean> future = new CompletableFuture<>();
+        client.publish(topic, payload, retain, qos).whenComplete((m, t) -> {


Why can't you use the clients future directly as return value? Maybe add the (unnecessary) boolean argument to the future to make it compatible?

This conversion stuff happening here right now is probably more inefficient than carrying a useless boolean in the type signature.

The clients return different futures: CompletableFuture<Mqtt3Publish> and CompletableFuture<Mqtt5Publish>.

...port.mqtt/src/main/java/org/eclipse/smarthome/io/transport/mqtt/internal/ClientCallback.java

...qtt/src/main/java/org/eclipse/smarthome/io/transport/mqtt/sslcontext/SSLContextProvider.java

davidgraeff

Looks good so far, one last inline comment from my side

...ansport.mqtt/src/main/java/org/eclipse/smarthome/io/transport/mqtt/MqttBrokerConnection.java

J-N-K · 2019-11-03T15:57:03Z

@openhab/core-maintainers If we postpone this again, we‘ll be too close to the next milestone to merge it. And I would feel uncomfortable to add this to a release without having it tested in a milestone before.

kaikreuzer · 2019-11-03T16:08:03Z

I'm ok with merging, but I would like to see @davidgraeff's final approval on it.
Plus, it seems you need to rebase :-)

davidgraeff

LGTM

cweitkamp

Thanks you for the work. I am afraid I cannot say much about the topic itself but I added some code style comments. Feel free to resolve them.

...ansport.mqtt/src/main/java/org/eclipse/smarthome/io/transport/mqtt/MqttBrokerConnection.java

cweitkamp · 2019-11-03T19:24:12Z

...ansport.mqtt/src/main/java/org/eclipse/smarthome/io/transport/mqtt/MqttBrokerConnection.java

+                if (t == null) {
+                    future.complete(true);
+                } else {
+                    logger.info("Error subscribing to topic {}", topic, t);


If this is an error, why not log an error?

I changed the wording and upgraded to warn. error is too much IMO, this does not affect the funtion of the other topics or the connection.

...ansport.mqtt/src/main/java/org/eclipse/smarthome/io/transport/mqtt/MqttBrokerConnection.java

...qtt/src/main/java/org/eclipse/smarthome/io/transport/mqtt/ssl/CustomTrustManagerFactory.java

cweitkamp · 2019-11-03T19:35:55Z

...ain/java/org/eclipse/smarthome/io/transport/mqtt/internal/client/MqttAsyncClientWrapper.java

+            case 2:
+                return MqttQos.EXACTLY_ONCE;
+            default:
+                throw new IllegalArgumentException();


Does it make sense to use a specific message for this exception?

...in/java/org/eclipse/smarthome/io/transport/mqtt/internal/client/Mqtt5AsyncClientWrapper.java

...in/java/org/eclipse/smarthome/io/transport/mqtt/internal/client/Mqtt3AsyncClientWrapper.java

J-N-K · 2019-11-04T17:44:52Z

Please note that https://github.com/openhab/openhab2-addons/pull/6245 needs to be merged to keep the tests in addons running.

Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>

cweitkamp

LGTM. Thanks.

Signed-off-by: Jan N. Klug <jan.n.klug@rub.de> GitOrigin-RevId: 07d10db

J-N-K force-pushed the hivemq branch from 464b724 to e736c1b Compare October 17, 2019 18:23

J-N-K mentioned this pull request Oct 17, 2019

[mqtt] adjust to new mqtt client openhab/openhab-addons#6245

Merged

wborn mentioned this pull request Oct 19, 2019

MqttBrokerConnectionTests unstable #1139

Closed

kaikreuzer requested a review from davidgraeff October 19, 2019 23:32

davidgraeff reviewed Oct 20, 2019

View reviewed changes

davidgraeff reviewed Oct 22, 2019

View reviewed changes

...ansport.mqtt/src/main/java/org/eclipse/smarthome/io/transport/mqtt/MqttBrokerConnection.java Outdated Show resolved Hide resolved

davidgraeff approved these changes Nov 3, 2019

View reviewed changes

cweitkamp reviewed Nov 3, 2019

View reviewed changes

J-N-K added 11 commits November 4, 2019 18:48

move to hivemq client

c3a78eb

Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>

add constructor for version selection

07c20be

Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>

allow providing only qos or retain on publish

7d06065

Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>

fixes

39d7542

Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>

fix feature

a873620

Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>

align with other inject bundles

485e961

Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>

address review comments

dc2b8c4

Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>

some more review

ab85b72

Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>

remove deprecation

c037bf8

Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>

address review comments

d989b10

Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>

address review comments

dcde344

Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>

J-N-K force-pushed the hivemq branch from 2173435 to dcde344 Compare November 4, 2019 17:49

cweitkamp approved these changes Nov 4, 2019

View reviewed changes

cweitkamp added rebuild Triggers the Jenkins PR build and removed rebuild Triggers the Jenkins PR build labels Nov 4, 2019

kaikreuzer merged commit 07d10db into openhab:master Nov 5, 2019

cweitkamp added this to the 2.5 milestone Nov 5, 2019

cweitkamp added the enhancement An enhancement or new feature of the Core label Nov 5, 2019

cweitkamp changed the title ~~move MQTT client from Paho to HiveMQ~~ Changed MQTT client from Paho to HiveMQ Dec 2, 2019

cweitkamp changed the title ~~Changed MQTT client from Paho to HiveMQ~~ [mqtt] Changed MQTT client from Paho to HiveMQ Dec 3, 2019

kaikreuzer removed the enhancement An enhancement or new feature of the Core label Dec 13, 2019

wborn mentioned this pull request Sep 23, 2020

Remove deprecated MQTT transport code #1668

Merged

Rosi2143 pushed a commit to Rosi2143/openhab-core that referenced this pull request Dec 26, 2020

Added cubic metre (openhab#1125)

f49f20e

J-N-K deleted the hivemq branch February 9, 2021 17:10

splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 11, 2023

move MQTT client from Paho to HiveMQ (openhab#1125)

fd65cb7

Signed-off-by: Jan N. Klug <jan.n.klug@rub.de> GitOrigin-RevId: 07d10db

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mqtt] Changed MQTT client from Paho to HiveMQ #1125

[mqtt] Changed MQTT client from Paho to HiveMQ #1125

J-N-K commented Oct 13, 2019

J-N-K commented Oct 13, 2019 •

edited

Loading

wborn commented Oct 13, 2019 •

edited

Loading

davidgraeff left a comment

davidgraeff Oct 20, 2019

J-N-K Oct 20, 2019

davidgraeff Oct 20, 2019

J-N-K Oct 20, 2019

J-N-K Oct 21, 2019

davidgraeff Oct 22, 2019

J-N-K Oct 22, 2019 •

edited

Loading

davidgraeff Oct 22, 2019

davidgraeff Oct 20, 2019 •

edited

Loading

J-N-K Oct 20, 2019 •

edited

Loading

davidgraeff left a comment

J-N-K commented Nov 3, 2019

kaikreuzer commented Nov 3, 2019

davidgraeff left a comment

cweitkamp left a comment

cweitkamp Nov 3, 2019

J-N-K Nov 4, 2019

cweitkamp Nov 3, 2019

J-N-K commented Nov 4, 2019

cweitkamp left a comment

[mqtt] Changed MQTT client from Paho to HiveMQ #1125

[mqtt] Changed MQTT client from Paho to HiveMQ #1125

Conversation

J-N-K commented Oct 13, 2019

J-N-K commented Oct 13, 2019 • edited Loading

wborn commented Oct 13, 2019 • edited Loading

davidgraeff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

J-N-K Oct 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidgraeff Oct 20, 2019 • edited Loading

Choose a reason for hiding this comment

J-N-K Oct 20, 2019 • edited Loading

Choose a reason for hiding this comment

davidgraeff left a comment

Choose a reason for hiding this comment

J-N-K commented Nov 3, 2019

kaikreuzer commented Nov 3, 2019

davidgraeff left a comment

Choose a reason for hiding this comment

cweitkamp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

J-N-K commented Nov 4, 2019

cweitkamp left a comment

Choose a reason for hiding this comment

J-N-K commented Oct 13, 2019 •

edited

Loading

wborn commented Oct 13, 2019 •

edited

Loading

J-N-K Oct 22, 2019 •

edited

Loading

davidgraeff Oct 20, 2019 •

edited

Loading

J-N-K Oct 20, 2019 •

edited

Loading