NIFI-4118 First commit of RethinkDB put processor #1942

mans2singh · 2017-06-26T13:30:40Z

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?
[] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?

For documentation related changes:

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.

…ravis-ci

mans2singh · 2017-06-26T14:21:53Z

The integration tests failed because RethinkDB was not available. I've disabled the integration tests.

mans2singh · 2017-06-29T13:21:38Z

Hey Folks:

Can you please give me your feedback on this RethinkDB Put processor ?

Thanks

Mans

jvwing · 2017-07-04T18:51:37Z

Reviewing

jvwing · 2017-07-04T22:39:36Z

...rocessors/src/main/java/org/apache/nifi/processors/rethinkdb/AbstractRethinkDbProcessor.java

+    static final Relationship REL_SUCCESS = new Relationship.Builder().name("success")
+            .description("Sucessful FlowFiles are routed to this relationship").build();
+
+    static final Relationship REL_FAILURE = new Relationship.Builder().name("success")


Should this be named failure?

jvwing · 2017-07-04T22:46:22Z

@mans2singh, thanks for contributing this! I have a few general comments from a first pass:

The notice information for RethinkDB should be included in the nifi-assembly/NOTICE file.
The RethinkDB project styles it's name as "RethinkDB" with capitalized R, D, and B. I recommend we do the same in all of the user-visible processor naming, docs, property descriptions, etc.
The processor docs should include a link to the RethinkDB project.
Thanks for including a comprehensive test suite with solid code coverage. The integration tests also ran fine for me.

And I have a question about connection management. I don't have any experience with RethinkDB. A brief search for documentation suggests that the Java connection might be thread safe (RethinkDB Issue #1041). Is that your understanding?

…ed classes to match RethinkDB conventions, and updated notice files

mans2singh · 2017-07-05T03:37:57Z

Hi @jvwing -

Thanks for your comments and feedback. I've updated the code (added RethinkDB site link to the description, renamed classes to match RethinkDB conventions, updated notice files and corrected the relation) based on your review.

Regarding thread safety of the driver - as you pointed out, it is thread safe. Here is thread safety issue closed (rethinkdb/rethinkdb#5166) and the closed pull request (rethinkdb/rethinkdb#5292).

Please let me know if you have any additional observations/recommendations.

Thanks again for your thorough review.

Mans

jvwing

Thanks for the update, @mans2singh. Everything is looking pretty good, I just found the one test issue and the question about strings.

jvwing · 2017-07-09T00:44:07Z

...fi-rethinkdb-processors/src/main/java/org/apache/nifi/processors/rethinkdb/PutRethinkDB.java

+        } catch (Exception exception) {
+            getLogger().error("Failed to insert into RethinkDB due to {}",
+                    new Object[]{exception.getLocalizedMessage()}, exception);
+            flowFile = session.putAttribute(flowFile, RETHINKDB_ERROR_MESSAGE, exception.getMessage() + "");


I notice you use the + "" method to cast objects to strings in several places. Any special reason? It does not appear too much in the NiFi codebase, as opposed to String.valueOf().

Thanks for the suggestion - I've updated the code to use String.valueOf method.

jvwing · 2017-07-09T00:46:06Z

...hinkdb-processors/src/test/java/org/apache/nifi/processors/rethinkdb/ITPutRethinkDBTest.java

+        runner.run(1,true,true);
+
+        flowFiles = runner.getFlowFilesForRelationship(PutRethinkDB.REL_FAILURE);
+        assertEquals(flowFiles.get(1).getAttribute(PutRethinkDB.RETHINKDB_INSERT_RESULT_DELETED_KEY), "0");


This integration test failed for me. I believe the failing flowfile is now at index 0, since REL_FAILURE is separate.

You are right - it was an oversight on my part. I've corrected the test.

mans2singh · 2017-07-09T01:37:20Z

@jvwing -

I've updated the code based on your comments. Please let me know if there is anything else required.

Thanks again.

Mans

joewitt · 2017-07-09T02:03:16Z

nifi-assembly/NOTICE

+
+      Copyright 2010-2012 RethinkDB
+
+      Licensed under the Apache License, Version 2.0 (the "License");


Lines 1035-1045 should be removed. See other ASLv2 dependencies as examples.

Removed the extra lines.

joewitt · 2017-07-09T02:09:57Z

nifi-nar-bundles/nifi-rethinkdb-bundle/nifi-rethinkdb-nar/src/main/resources/META-INF/NOTICE

+    RethinkDB Language Drivers
+
+    Copyright 2010-2012 RethinkDB
+


lines 18-29 can be removed. Also need to add 'commons-io' in here. Can copy it from the top level NOTICE that has it.

Removed extra lines and added commons-io as recommended.

mans2singh · 2017-07-09T02:25:38Z

@joewitt @jvwing -

Thanks for your review feedback and advice. I've updated the notices as you had mentioned.

Let me know if you have any other thoughts/recommendations.

Mans

jvwing · 2017-07-09T18:26:24Z

Thanks @mans2singh! This has been merged in.

mans2singh · 2017-07-09T22:04:12Z

Thanks @jvwing

mans2singh added 2 commits June 25, 2017 18:53

NIFI-4118 First commit of RethinkDB put processor

21fe145

Disabled integration tests due to RethinkDB not being available for t…

e0a6d10

…ravis-ci

jvwing reviewed Jul 4, 2017

View reviewed changes

mans2singh added 2 commits July 4, 2017 20:24

NIFI-4118 - Added RethinkDB site link, corrected relation name, renam…

b0fdad4

…ed classes to match RethinkDB conventions, and updated notice files

Updated description to use RethinkDB name

efb723a

jvwing reviewed Jul 9, 2017

View reviewed changes

mans2singh added 2 commits July 8, 2017 18:28

NIFI-4118 - Corrected string concatenation and failing integration tests

e090c34

NIFI-4118 - Use String.valueOf based on review comments

202e687

joewitt reviewed Jul 9, 2017

View reviewed changes

mans2singh added 2 commits July 8, 2017 19:12

NIFI-4118 - Corrected notices

cf71993

NIFI-4118 Added commons-io license

5a817c3

asfgit closed this in affc88e Jul 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NIFI-4118 First commit of RethinkDB put processor #1942

NIFI-4118 First commit of RethinkDB put processor #1942

mans2singh commented Jun 26, 2017 •

edited

mans2singh commented Jun 26, 2017

mans2singh commented Jun 29, 2017

jvwing commented Jul 4, 2017

jvwing Jul 4, 2017

jvwing commented Jul 4, 2017

mans2singh commented Jul 5, 2017 •

edited

jvwing left a comment

jvwing Jul 9, 2017

mans2singh Jul 9, 2017 •

edited

jvwing Jul 9, 2017

mans2singh Jul 9, 2017

mans2singh commented Jul 9, 2017

joewitt Jul 9, 2017

mans2singh Jul 9, 2017

joewitt Jul 9, 2017

mans2singh Jul 9, 2017

mans2singh commented Jul 9, 2017 •

edited

jvwing commented Jul 9, 2017

mans2singh commented Jul 9, 2017


		Copyright 2010-2012 RethinkDB

		Licensed under the Apache License, Version 2.0 (the "License");

NIFI-4118 First commit of RethinkDB put processor #1942

NIFI-4118 First commit of RethinkDB put processor #1942

Conversation

mans2singh commented Jun 26, 2017 • edited

For all changes:

For code changes:

For documentation related changes:

Note:

mans2singh commented Jun 26, 2017

mans2singh commented Jun 29, 2017

jvwing commented Jul 4, 2017

Choose a reason for hiding this comment

jvwing commented Jul 4, 2017

mans2singh commented Jul 5, 2017 • edited

jvwing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mans2singh Jul 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mans2singh commented Jul 9, 2017

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

mans2singh commented Jul 9, 2017 • edited

jvwing commented Jul 9, 2017

mans2singh commented Jul 9, 2017

mans2singh commented Jun 26, 2017 •

edited

mans2singh commented Jul 5, 2017 •

edited

mans2singh Jul 9, 2017 •

edited

mans2singh commented Jul 9, 2017 •

edited