NIFI-10411 Add record processing feature to PublishMQTT processor #6373
Conversation
There was a problem hiding this comment.
@nandorsoma Thanks for adding this new feature in PublishMQTT!
I tested it with different inputs (valid / invalid records) and it works as expected. The prod code looks good to me but I haven't reviewed the tests yet.
I added a comment about the descriptions of Record Reader/Writer properties and one more about the Json library being used.
Will look into the tests in more detail soon.
...t-processors/src/main/java/org/apache/nifi/processors/mqtt/common/AbstractMQTTProcessor.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
@nandorsoma Reviewed the tests and only found some minor improvement points. Could you please check my comments? Thanks
...ndle/nifi-mqtt-processors/src/test/java/org/apache/nifi/processors/mqtt/TestPublishMQTT.java
Outdated
Show resolved
Hide resolved
...ndle/nifi-mqtt-processors/src/test/java/org/apache/nifi/processors/mqtt/TestPublishMQTT.java
Outdated
Show resolved
Hide resolved
...ndle/nifi-mqtt-processors/src/test/java/org/apache/nifi/processors/mqtt/TestConsumeMQTT.java
Outdated
Show resolved
Hide resolved
|
Thank you for your reviews @turcsanyip and @exceptionfactory! Please see my latest commit! |
| topic = "testTopic"; | ||
| testRunner.setProperty(PublishMQTT.PROP_TOPIC, topic); | ||
| private static ArrayNode createTestJsonInput() { | ||
| final ObjectMapper mapper = new ObjectMapper(); |
There was a problem hiding this comment.
I think it is a best practice to initialize ObjectMapper once and store it in a static field because it seems to be a heavy-weight object but thread-safe.
It can be modified in a future commit. Will go ahead with merging this PR.
There was a problem hiding this comment.
@nandorsoma Thanks for the review changes!
@exceptionfactory Thanks for your recommendation on json libraries! The code has been modified to use Jackson.
Added a comment about a possible minor improvement that can be implemented later.
+1 LGTM
Merging to main.
This closes apache#6373. Signed-off-by: Peter Turcsanyi <turcsanyi@apache.org>
Summary
NIFI-10411
This pr adds record processing feature to PublishMQTT processor. I also made a little bit of cleanup around the tests because the current structure is no longer useful (in the past multiple Test extended the Common one) and now it just makes the code difficult to understand.
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000NIFI-00000Pull Request Formatting
mainbranchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
mvn clean install -P contrib-checkLicensing
LICENSEandNOTICEfilesDocumentation