-
Notifications
You must be signed in to change notification settings - Fork 782
Conversation
Hi @davidgraeff! This is a feature I was hoping for since a long while 👍 So regarding your discussion point: I would clearly say that this can go into ESH, especially as Moquette once almost managed to become an Eclipse project as well. I wouldn't call this a "binding" through (this is where you have currently put it). I'd think the better place would be https://github.com/eclipse/smarthome/tree/master/extensions/io, i.e. calling it "some I/O related extension". |
259b1d5
to
684644a
Compare
684644a
to
23234ab
Compare
@kaikreuzer Just to be on the same side here:
|
Sound good 👍 |
23234ab
to
c58126f
Compare
I might be a little late to the party, but hopefully you don't mind asking this question anyway: Why exactly do you all think it makes sense to have an embedded MQTT server within ESH? I mean, there are brilliant, easy-to-setup standalone brokers out there, working nicely as a standalone service. Isn't the approach of embedding it pretty much against the micro service idea of doing one thing (and doing it good) as opposed to the old JEE world of putting everything into the same process? |
I agree that an Mqtt server should not be part of the core and nothing should rely on this and it should not run in the same process. This is why this bundle is as independent as possible and the MQTT server runs in its own process. Because Mqtt is a key protocol and only works with a server, it might be a good idea to provide a out of the box working a good solution here. Mqtt can be used for ESH instance sycronisation for example. I see this bundle as extension. like any other extensions, it is discussable if it should be in the core repository. |
But that is the problem: These are external services that you need to setup and configure accordingly. You cannot easily build integrations using MQTT as you are always just a client, but not the server. Like supporting sensors that use MQTT and that need to be told where to connect to (=the ESH instance).
Microservices in an OSGi sense are services WITHIN the same JVM - and that's the approach here. Note that the feature discussed here is merely to wrap an existing broker, not to implement any on our own. And as @davidgraeff says, it is a fully optional component that nobody is required to use - but imho it can be tremendously helpful for many. |
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.
Hi @davidgraeff,
What is the status of this PR? Are you waiting for any discussions/decisions? Imho, this PR should be pretty unrelated to most of the rest and mergable on its own, do you agree?
* | ||
* @author David Graeff - Initial contribution | ||
*/ | ||
@Component(immediate = true, configurationPid = { "mqttembeddedbroker" }, property = { |
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.
pid should always be prefixed by "org.eclipse.smarthome."
<artifactId>org.eclipse.smarthome.io.mqttembeddedbroker</artifactId> | ||
<version>0.9.0-SNAPSHOT</version> | ||
|
||
<name>MqttBroker Binding</name> |
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 is not a binding.
Change to "Embedded MQTT Broker"
@@ -0,0 +1,10 @@ | |||
package org.eclipse.smarthome.io.mqttembeddedbroker.internal; |
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.
license header missing
@@ -0,0 +1,10 @@ | |||
package org.eclipse.smarthome.io.mqttembeddedbroker.internal; | |||
|
|||
public class ServiceConfiguration { |
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.
javadoc missing
@@ -0,0 +1,32 @@ | |||
# MqttEmbeddedBroker Binding |
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 is not a binding.
Change it to "Embedded MQTT Broker"
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.
@davidgraeff ping :-)
* __secure__: If set, hosts a secure SSL connection on port 8883 or otherwise a non secure connection on port 1883 (if not overwritten by the port parameter). | ||
* __persistence_file__: The file were messages are stored for a persistence feature. Can be empty. Defaults to _moquette.msgs_. | ||
|
||
## Channels |
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.
remove
|
||
There are no channels available. The status message and possible reasons for a failed connection can be accessed through thing properties. The broker connection is successful if the thing is ONLINE. | ||
|
||
## Full Example |
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.
remove
Fragment-Host: org.eclipse.smarthome.io.mqttembeddedbroker | ||
Import-Package: org.eclipse.smarthome.core.common.registry, | ||
org.eclipse.smarthome.core.events, | ||
org.eclipse.smarthome.core.thing, |
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.
There should be no dependency on org.eclipse.smarthome.core.thing
org.eclipse.smarthome.config.core, | ||
org.eclipse.smarthome.config.discovery, | ||
org.eclipse.smarthome.core.library.types, | ||
org.eclipse.smarthome.core.thing, |
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.
There should be no dependency on org.eclipse.smarthome.core.thing.*
org.eclipse.jdt.annotation;resolution:=optional, | ||
org.osgi.service.component.annotations;resolution:=optional, | ||
org.eclipse.smarthome.config.core, | ||
org.eclipse.smarthome.config.discovery, |
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.
There should be no dependency on org.eclipse.smarthome.config.discovery.
@kaikreuzer I'm in Japan and cannot contribute code at the moment. All my PRs are on hold. |
I assume you are back, at least I have seen you recently ;-) |
@davidgraeff Friendly ping! |
@davidgraeff Another friendly ping. |
Unfortunate timing that we didn't finish those PRs before I went for another traveling journey but I'm back end of the week. |
@davidgraeff Did you find any time for it? |
@davidgraeff Another friendly ping! |
@davidgraeff As you are active tonight: Any chance to finally address this? |
It's a 12h/d work week atm, but yes I want this to be included as well. I just first addressed all OpenHab issues and PRs on my todo list. |
8787bd6
to
17e3fe1
Compare
17e3fe1
to
7324c5f
Compare
7324c5f
to
c0fc95d
Compare
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.
Some comments inline.
@@ -0,0 +1,32 @@ | |||
# MqttEmbeddedBroker Binding |
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.
@davidgraeff ping :-)
@@ -0,0 +1,32 @@ | |||
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" |
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.
drop about.html, replace by NOTICE file.
OSGI-INF/,\ | ||
ESH-INF/,\ | ||
lib/*.jar,\ | ||
about.html |
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.
replace with NOTICE
*/ | ||
public void stopBrokerStartDetection() { | ||
if (schedule != null) { | ||
schedule.cancel(true); |
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.
should schedule also be set to null 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.
doesn't hurt, I've added that
* @author David Graeff - Initial contribution | ||
*/ | ||
public class MqttEmbeddedBrokerMetrics implements InterceptHandler { | ||
public interface BrokerMetricsListener { |
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.
please JavaDoc the interface and method.
* | ||
*/ | ||
public interface MqttEmbeddedBrokerStartedListener { | ||
public void mqttEmbeddedBrokerStarted(boolean timeout); |
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.
please add JavaDoc for the method.
public Cluster getCluster() { | ||
return null; | ||
} | ||
} |
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.
missing new-line
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.
done
@htreu Done |
Bundle-Name: MqttBroker Binding Tests | ||
Bundle-SymbolicName: org.eclipse.smarthome.io.mqttembeddedbroker.test;singleton:=true | ||
Bundle-Vendor: Eclipse.org/SmartHome | ||
Bundle-Version: 0.9.0.qualifier |
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.
nedds to be updated
Bundle-Name: Embedded Mqtt Broker | ||
Bundle-SymbolicName: org.eclipse.smarthome.io.mqttembeddedbroker;singleton:=true | ||
Bundle-Vendor: Eclipse.org/SmartHome | ||
Bundle-Version: 0.9.0.qualifier |
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.
nedds to be updated
org.eclipse.smarthome.io.transport.mqtt, | ||
org.eclipse.smarthome.io.transport.mqtt.reconnect, | ||
org.eclipse.smarthome.io.transport.mqtt.sslcontext, | ||
org.osgi.framework;version="1.8.0", |
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 version constraint needs to be removed.
org.eclipse.smarthome.io.transport.mqtt.sslcontext, | ||
org.osgi.framework;version="1.8.0", | ||
org.osgi.service.component, | ||
org.osgi.service.component.annotations;resolution:=optional, |
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 assume this should be removed: https://github.com/openhab/openhab2-addons/pull/2878#discussion_r152942296
org.osgi.service.component, | ||
org.osgi.service.component.annotations;resolution:=optional, | ||
org.slf4j | ||
Comment: TODO Add org.eclipse.smarthome.io.net.security.api for secure connections |
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 don't think this should be placed in the manifest if in production (merged)
@@ -20,6 +20,7 @@ Import-Package: | |||
org.eclipse.smarthome.io.transport.mqtt.reconnect, | |||
org.eclipse.smarthome.io.transport.mqtt.sslcontext, | |||
org.osgi.framework, | |||
org.osgi.service.cm;version="1.5.0", |
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.
please remove the version restriction.
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.
Can I teach Eclipse not to add these automatically? Would be awesome
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.
only one last minute missing NOTICE file.
Otherwise this looks good to me. After approval we need to file a CQ since this exceeds 1k lines.
Also moquette and netty need CQs.
src/test/resources/ | ||
output.. = target/test-classes | ||
bin.includes = META-INF/,\ | ||
. |
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.
sorry, didn't notice before: NOTICE file is missing.
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.
done
Thanks @davidgraeff! I suggest to freeze this now and file the appropriate CQs. I will do so as soon as privileges are granted to my by eclipse.org. |
All right. |
@davidgraeff since netty-4.1.19 was the most recent version approved by the eclipse IP team please update the version used here to 4.1.19 (instead of 4.1.13). The netty change log lists a lot of bug fixes for this version bump. |
Moquette 0.10 has some memory leak issues (https://github.com/andsel/moquette/issues/385). It should be upgraded as well as soon as 0.11 is out. |
Thanks @davidgraeff. Lets first see what the eclipse IP team can make out of the 0.10 version. FYI: netty CQ16091 is already approved. |
Thanks for pointing that out. Adding something that is already known to eat memory sounds a little bit weird. |
@maggu2810 Netty is just used to convert the received/published messages so publishing/receiving causes memory leaks and not concurrent connections. |
CQ16090 for moquette is still not approved but we have clearance to proceed with the checkin. I will start a last review before issuing a CQ for the actual code. |
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.
Hi @davidgraeff I found some remaining issue. See below and inline. Thanks.
Please remove the lib/moquette-broker-src-0.10.zip
since we have a binary-only CQ approved.
When I finally got this to run the following log entries appear:
08:34:17.891 WARN i.m.s.DefaultMoquetteSslContextCreator[:54] - The keystore path is null or empty. The SSL context won't be initialized.
08:34:17.891 ERROR i.m.server.netty.NettyAcceptor[:164] - Can't initialize SSLHandler layer! Exiting, check your configuration of jks
which leads to a stopped moquette broker.
@@ -0,0 +1,20 @@ | |||
Manifest-Version: 1.0 | |||
Bundle-ManifestVersion: 2 | |||
Bundle-Name: MqttBroker Binding Tests |
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.
one obsolete " " here
<bundle.namespace>org.eclipse.smarthome.io.mqttembeddedbroker</bundle.namespace> | ||
</properties> | ||
|
||
|
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.
empty line
@@ -0,0 +1,144 @@ | |||
/** | |||
* Copyright (c) 2010-2017 by the respective copyright holders. |
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.
please update all license headers to EPL 2
Use the command mvn license:format -pl extensions/io/org.eclipse.smarthome.io.mqttembeddedbroker.test
from the project root.
public class MqttEmbeddedBrokerServiceTest { | ||
|
||
// Create an accept all trust manager for a client SSLContext | ||
private static class X509ExtendedTrustManagerEx extends X509ExtendedTrustManager { |
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 class seems to be unused right now, please add a @SuppressWarnings("unused")
so the whole test project is free of warnings.
} | ||
} | ||
|
||
EmbeddedBrokerService subject; |
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.
should be private
* | ||
* @author David Graeff - Initial contribution | ||
*/ | ||
@Component(immediate = true, service = EmbeddedBrokerService.class, configurationPid = { |
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.
configurationPid
must not be a String[]
public MqttBrokerConnection connection; | ||
|
||
@Reference(cardinality = ReferenceCardinality.MANDATORY) | ||
public void setMqttService(MqttService service) { |
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.
please also add the unbind method
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 thought, an unbind method is not required if ReferenceCardinality.MANDATORY
is set, because the entire service is teared down if the mandatory referenced service died.
Bundle-Vendor: Eclipse.org/SmartHome | ||
Bundle-Version: 0.10.0.qualifier | ||
Bundle-RequiredExecutionEnvironment: JavaSE-1.8 | ||
Bundle-ClassPath: ., lib/moquette-broker-0.10.jar, lib/netty-all-4.1.13.Final.jar |
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.
its netty 4.1.19 now
.,\ | ||
OSGI-INF/,\ | ||
ESH-INF/,\ | ||
lib/*.jar,\ |
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.
with the moquette source zip removed this can be changed to lib/
|
||
This bundle allows to configure and start an embedded MQTT Server (based on (Moquette)[https://github.com/andsel/moquette]) for an easy way to have an MQTT Server up and running with a click. | ||
|
||
## Supported Things |
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 bundle now adds a service, please update the docs accordingly.
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.
Thanks @davidgraeff. I will create the corresponding CQ. Please do not change this PR anymore.
This io/extension can add a Mqtt broker connection and start an embedded broker (Moquette). It allows secure connection via a provided and configured certificate or creates a self-signed certificate and key on the first start otherwise. Signed-off-by: David Graeff <david.graeff@web.de>
FTR: all commits squashed and CQ16166 created. edit: link removed. |
CQ16166 got approved. Ready to go. |
This io/extension can add a Mqtt broker connection and start an embedded broker (Moquette). It allows secure connection via a provided and configured certificate or creates a self-signed certificate and key on the first start otherwise. Signed-off-by: David Graeff <david.graeff@web.de>
This is an extension to PR #3839 (Mqtt Broker configuration).
Abstract
Mqtt can be seen as an important IoT protocol next to CoAP but with different goals and a different architecture. CoAP is supported within ESH with the Californium library and at the moment bindings just use the raw library without any transport abstraction. Which is fine for CoAP. Mqtt on the other side needs a central additional component, the Mqtt broker. This binding lowers the entry barrier into the Mqtt world and allows to start a secure, correctly configured Mqtt server with a button click.
Features
This bundle contains an embedded broker (Moquette) and will create a
MqttBrokerConnection
instance shared with theMqttService
so that other bindings, especially the MQTT Broker Binding can use the Broker.Plain tcp and also secure ssl connection (with #4105) are possible for external clients, depending on the configuration.
The service is configured via ConfigAdmin but implements ConfigurableService to be configurable via PaperUI as well.
3rd party licenses
The Moquette server is licensed as EPL 1.0 / Apache 2.0, the required jetty tcp server is licensed as Apache 2.0.