You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by SebastianCarroll <gi...@git.apache.org> on 2017/09/13 15:29:07 UTC
[GitHub] nifi pull request #2154: NIFI-2663: Add WebSocket support for MQTT processor...
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 <sc...@hortonworks.com>
Date: 2017-09-13T15:21:56Z
NIFI-2663: Add WebSocket support for MQTT processors
----
---
[GitHub] nifi issue #2154: NIFI-2663: Add WebSocket support for MQTT processors
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on the issue:
https://github.com/apache/nifi/pull/2154
Hello Sebastian, yup that subclassing strategy makes sense.
---
[GitHub] nifi issue #2154: NIFI-2663: Add WebSocket support for MQTT processors
Posted by SebastianCarroll <gi...@git.apache.org>.
Github user SebastianCarroll commented on the issue:
https://github.com/apache/nifi/pull/2154
I've added an abstract `getUri` method which allows me to subclass `TestConsumeMQTT` -> `TestConsumeMqttWS` and `TestConsumeMqttSSL` -> `TestConsumeMqttWSS` and only override the getUri method to return the `ws://` and `wss://` protocols respectively but still run the same set of tests for the base classes.
This approach is currently failing with the error `java.io.IOException: WebSocket Response header: Invalid response from Server, It may not support WebSockets.`. Initially I thought that the Moquette server may not support WebSockets but on closer inspection it looks like a misconfiguration.
Putting aside the current test failures, does this subclassing test strategy make sense?
---
[GitHub] nifi issue #2154: NIFI-2663: Add WebSocket support for MQTT processors
Posted by JPercivall <gi...@git.apache.org>.
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.`
---
[GitHub] nifi pull request #2154: NIFI-2663: Add WebSocket support for MQTT processor...
Posted by joewitt <gi...@git.apache.org>.
Github user joewitt commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2154#discussion_r143278549
--- Diff: nifi-nar-bundles/nifi-mqtt-bundle/nifi-mqtt-processors/src/test/java/org/apache/nifi/processors/mqtt/common/MqttTestClient.java ---
@@ -195,4 +218,296 @@ public String getServerURI() {
public void close() throws MqttException {
}
+
--- End diff --
There is a lot of boilerplate looking javadocs. Are these copied/pasted from somewhere? Where is that?
---
[GitHub] nifi issue #2154: NIFI-2663: Add WebSocket support for MQTT processors
Posted by SebastianCarroll <gi...@git.apache.org>.
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
---
[GitHub] nifi pull request #2154: NIFI-2663: Add WebSocket support for MQTT processor...
Posted by SebastianCarroll <gi...@git.apache.org>.
Github user SebastianCarroll commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2154#discussion_r143336390
--- Diff: nifi-nar-bundles/nifi-mqtt-bundle/nifi-mqtt-processors/src/test/java/org/apache/nifi/processors/mqtt/common/MqttTestClient.java ---
@@ -195,4 +218,296 @@ public String getServerURI() {
public void close() throws MqttException {
}
+
--- End diff --
Hey @joewitt! The newer version adds methods to the IMqttClient interface and I auto-imported them. It pulled in the JavaDocs as well (could be the IDEA default?) and I originally left them as I was sure if they were required. I have removed them in the new update as it seems unnecessary to include them for the MqttTestClient. Would you agree?
---
[GitHub] nifi issue #2154: NIFI-2663: Add WebSocket support for MQTT processors
Posted by SebastianCarroll <gi...@git.apache.org>.
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
---