Uploaded image for project: 'Apache NiFi'
  1. Apache NiFi
  2. NIFI-2663

Add Websocket support for MQTT protocol

    Details

    • Type: Improvement
    • Status: Open
    • Priority: Major
    • Resolution: Unresolved
    • Affects Version/s: 1.0.0, 0.7.0, 0.6.1
    • Fix Version/s: None
    • Component/s: Core Framework
    • Labels:
      None

      Description

      Today NiFi only supports MQTT over plain TCP using the PublishMQTT and ConsumeMQTT processors. However, there are many cases in the IoT world where WebSockets (secure and not) is the preferred transport mechanism for MQTT. This JIRA is to enhance those processors to also support WS.

      Following are the basics of what is required:

      1) Require to configure web socket as the transport protocol, currently PublishMQTT processor supports TCP as the only transport protocol
      2) URL input for the Web socket transport should be in the format of ws://IPAddress:websocket_listening_port, as of now it accepts only TCP url, and a bulletin is raised if a protocol other than tcp or ssl is used.
      3) Similar to TCP non-secured and secured publisher we should extend/provide processor to support WS and WSS transport protocols

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user SebastianCarroll opened a pull request:

          https://github.com/apache/nifi/pull/2154

          NIFI-2663: Add WebSocket support for MQTT processors

          Thank you for submitting a contribution to Apache NiFi.

          In order to streamline the review of the contribution we ask you
          to ensure the following steps have been taken:

              1. For all changes:
          • [x] Is there a JIRA ticket associated with this PR? Is it referenced
            in the commit message?
          • [x] Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
          • [x] Has your PR been rebased against the latest commit within the target branch (typically master)?
          • [x] Is your initial contribution a single, squashed commit?
              1. For code changes:
          • [x] Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
          • [ ] Have you written or updated unit tests to verify your changes?
          • [x] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
          • [x] If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
          • [x] If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
          • [x] If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?
              1. For documentation related changes:
          • [x] Have you ensured that format looks appropriate for the output in which it is rendered?
              1. Note:
                Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/SebastianCarroll/nifi NIFI-2663

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/nifi/pull/2154.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #2154


          commit 4f6184f966fd19b2d892f43417f42635107a1da7
          Author: Sebastian Carroll <scarroll@hortonworks.com>
          Date: 2017-09-13T15:21:56Z

          NIFI-2663: Add WebSocket support for MQTT processors


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user SebastianCarroll opened a pull request: https://github.com/apache/nifi/pull/2154 NIFI-2663 : Add WebSocket support for MQTT processors Thank you for submitting a contribution to Apache NiFi. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: For all changes: [x] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? [x] Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character. [x] Has your PR been rebased against the latest commit within the target branch (typically master)? [x] Is your initial contribution a single, squashed commit? For code changes: [x] Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder? [ ] Have you written or updated unit tests to verify your changes? [x] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0] ( http://www.apache.org/legal/resolved.html#category-a)? [x] If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly? [x] If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly? [x] If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties? For documentation related changes: [x] Have you ensured that format looks appropriate for the output in which it is rendered? Note: Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible. You can merge this pull request into a Git repository by running: $ git pull https://github.com/SebastianCarroll/nifi NIFI-2663 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/nifi/pull/2154.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #2154 commit 4f6184f966fd19b2d892f43417f42635107a1da7 Author: Sebastian Carroll <scarroll@hortonworks.com> Date: 2017-09-13T15:21:56Z NIFI-2663 : Add WebSocket support for MQTT processors
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user SebastianCarroll commented on the issue:

          https://github.com/apache/nifi/pull/2154

          I'm not sure what the best way to handle the unit tests addition would be. I could just copy and paste the unit tests that are there, but change ssl -> wss and tcp -> ws but that seems like a lot of duplication of code for a 3 character change

          Show
          githubbot ASF GitHub Bot added a comment - Github user SebastianCarroll commented on the issue: https://github.com/apache/nifi/pull/2154 I'm not sure what the best way to handle the unit tests addition would be. I could just copy and paste the unit tests that are there, but change ssl -> wss and tcp -> ws but that seems like a lot of duplication of code for a 3 character change
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user JPercivall commented on the issue:

          https://github.com/apache/nifi/pull/2154

          @SebastianCarroll the way the test cases are set up you don't need to copy and paste the tests themselves. What you'll do is extend common/Test*MqttCommon in order to get all the junit tests. See [TestPublishMQTT](https://github.com/apache/nifi/blob/7923fd04c35737df8145b213536bdf333ef72713/nifi-nar-bundles/nifi-mqtt-bundle/nifi-mqtt-processors/src/test/java/org/apache/nifi/processors/mqtt/integration/TestPublishMQTT.java#L38) in the integration folder as an example.

          While it may be just a 3 letter change on our end, you're changing a core configuration of the underlying implementation. So verifying that is helpful.

          Also, what is the purpose of the changes you've made to MqttTestClient.java and where did you copy & paste the comments/code from?

          Lastly, you have some check style issues (can be seen by running "mvn clean install -Pcontrib-check"). Here's what I see:

          `[WARNING] src/main/java/org/apache/nifi/processors/mqtt/common/AbstractMQTTProcessor.java[104:52] (whitespace) OperatorWrap: '||' should be on a new line.
          [WARNING] src/main/java/org/apache/nifi/processors/mqtt/common/AbstractMQTTProcessor.java[105:53] (whitespace) OperatorWrap: '||' should be on a new line.
          [WARNING] src/main/java/org/apache/nifi/processors/mqtt/common/AbstractMQTTProcessor.java[106:52] (whitespace) OperatorWrap: '||' should be on a new line.
          [WARNING] src/main/java/org/apache/nifi/processors/mqtt/common/AbstractMQTTProcessor.java[296:62] (whitespace) OperatorWrap: '||' should be on a new line.
          [WARNING] src/test/java/org/apache/nifi/processors/mqtt/common/MqttTestClient.java[20] (imports) AvoidStarImport: Using the '.' form of import should be avoided - org.eclipse.paho.client.mqttv3..
          [WARNING] src/test/java/org/apache/nifi/processors/mqtt/common/MqttTestClient.java[375:43] (whitespace) FileTabCharacter: Line contains a tab character.
          [WARNING] src/test/java/org/apache/nifi/processors/mqtt/common/MqttTestClient.java[476:43] (whitespace) FileTabCharacter: Line contains a tab character.`

          Show
          githubbot ASF GitHub Bot added a comment - Github user JPercivall commented on the issue: https://github.com/apache/nifi/pull/2154 @SebastianCarroll the way the test cases are set up you don't need to copy and paste the tests themselves. What you'll do is extend common/Test*MqttCommon in order to get all the junit tests. See [TestPublishMQTT] ( https://github.com/apache/nifi/blob/7923fd04c35737df8145b213536bdf333ef72713/nifi-nar-bundles/nifi-mqtt-bundle/nifi-mqtt-processors/src/test/java/org/apache/nifi/processors/mqtt/integration/TestPublishMQTT.java#L38 ) in the integration folder as an example. While it may be just a 3 letter change on our end, you're changing a core configuration of the underlying implementation. So verifying that is helpful. Also, what is the purpose of the changes you've made to MqttTestClient.java and where did you copy & paste the comments/code from? Lastly, you have some check style issues (can be seen by running "mvn clean install -Pcontrib-check"). Here's what I see: ` [WARNING] src/main/java/org/apache/nifi/processors/mqtt/common/AbstractMQTTProcessor.java [104:52] (whitespace) OperatorWrap: '||' should be on a new line. [WARNING] src/main/java/org/apache/nifi/processors/mqtt/common/AbstractMQTTProcessor.java [105:53] (whitespace) OperatorWrap: '||' should be on a new line. [WARNING] src/main/java/org/apache/nifi/processors/mqtt/common/AbstractMQTTProcessor.java [106:52] (whitespace) OperatorWrap: '||' should be on a new line. [WARNING] src/main/java/org/apache/nifi/processors/mqtt/common/AbstractMQTTProcessor.java [296:62] (whitespace) OperatorWrap: '||' should be on a new line. [WARNING] src/test/java/org/apache/nifi/processors/mqtt/common/MqttTestClient.java [20] (imports) AvoidStarImport: Using the '. ' form of import should be avoided - org.eclipse.paho.client.mqttv3. . [WARNING] src/test/java/org/apache/nifi/processors/mqtt/common/MqttTestClient.java [375:43] (whitespace) FileTabCharacter: Line contains a tab character. [WARNING] src/test/java/org/apache/nifi/processors/mqtt/common/MqttTestClient.java [476:43] (whitespace) FileTabCharacter: Line contains a tab character.`
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user SebastianCarroll commented on the issue:

          https://github.com/apache/nifi/pull/2154

          Hey @JPercivall! Thanks for the review! What I was referring to with the 3 letter change was in the test class itself not the core code. For example, I thought about changing the 'ssl' value in [TestConsumeMqttSSL](https://github.com/apache/nifi/blob/7923fd04c35737df8145b213536bdf333ef72713/nifi-nar-bundles/nifi-mqtt-bundle/nifi-mqtt-processors/src/test/java/org/apache/nifi/processors/mqtt/integration/TestConsumeMqttSSL.java#L74) assuming that we would want to run all the same tests for wss as to ssl (and similarly for ws as for tcp). Subclassing common/Test*MqttCommon as you suggest won't remove this issue as I would essentially be reimplementing all the tests and setup from TestConsumeMqttSSL. However I could subclass TestConsumeMqttSSL -> TestConsumeMqttWss (for example) and pull the assigning of the broker out into a method and just overwrite that in the subclass.

          For the changes to MqttTestClient.java, they were autogenerated using Intellij. I believe this is due to changes in the IMqttClient interface referenced in the newer version of the Paho dependancy (1.1.1 from 1.0.2). Is it good practice to leave these as are? It does feel unnecessary to have all the comments there given they are only implemented to make a stub (or mock? I'm never sure exactly which is which)

          I'm still trying to get the -Pcontrib-check flag to work. I originally ticked that box in the PR erroneously.

          Hope this helps with the review and thanks again!
          Seb

          Show
          githubbot ASF GitHub Bot added a comment - Github user SebastianCarroll commented on the issue: https://github.com/apache/nifi/pull/2154 Hey @JPercivall! Thanks for the review! What I was referring to with the 3 letter change was in the test class itself not the core code. For example, I thought about changing the 'ssl' value in [TestConsumeMqttSSL] ( https://github.com/apache/nifi/blob/7923fd04c35737df8145b213536bdf333ef72713/nifi-nar-bundles/nifi-mqtt-bundle/nifi-mqtt-processors/src/test/java/org/apache/nifi/processors/mqtt/integration/TestConsumeMqttSSL.java#L74 ) assuming that we would want to run all the same tests for wss as to ssl (and similarly for ws as for tcp). Subclassing common/Test*MqttCommon as you suggest won't remove this issue as I would essentially be reimplementing all the tests and setup from TestConsumeMqttSSL. However I could subclass TestConsumeMqttSSL -> TestConsumeMqttWss (for example) and pull the assigning of the broker out into a method and just overwrite that in the subclass. For the changes to MqttTestClient.java, they were autogenerated using Intellij. I believe this is due to changes in the IMqttClient interface referenced in the newer version of the Paho dependancy (1.1.1 from 1.0.2). Is it good practice to leave these as are? It does feel unnecessary to have all the comments there given they are only implemented to make a stub (or mock? I'm never sure exactly which is which) I'm still trying to get the -Pcontrib-check flag to work. I originally ticked that box in the PR erroneously. Hope this helps with the review and thanks again! Seb

            People

            • Assignee:
              apsaltis Andrew Psaltis
              Reporter:
              apsaltis Andrew Psaltis
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:

                Development