Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NIFI-570: Added MongoDB put and get processors #53

Merged
merged 1 commit into from May 19, 2015

Conversation

Projects
None yet
6 participants
@tequalsme
Copy link
Contributor

commented May 8, 2015

No description provided.

@joewitt

This comment has been minimized.

Copy link
Contributor

commented May 8, 2015

Tim - this is really awesome for a couple reasons. First, adding support for MongoDB is cool. Second, I've only done the eyeball test but this looks great, well thought through, well written. It is great that you even updated the NOTICE. I believe though the copyright mention should be 'Copyright (C) 2008-2013 10gen, Inc.' as that is what they had in their LICENSE addendum section for the version you've chosen.

Is there any reason not to go straight to 3.0.x driver series?

Thanks!
Joe

@tequalsme

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2015

Good eye, Joe. I was about to comment on a couple of things for this pull request:

1 - I wasn't sure what exactly the verbiage should be in the NOTICE. I guessed based on their website, and it looks like I guessed wrong. I'll get it from their LICENSE and update.
2 - I am using embedmongo-maven-plugin for my unit tests. I want to make sure it's ok to introduce this new dependency, could someone validate? https://github.com/joelittlejohn/embedmongo-maven-plugin
3 - The 3.0 Java driver series was released a month ago and has a brand new API (that I have not used). The 2.x API is still supported but marked deprecated. I though it best to go with 2.x than to introduce a slew of deprecation warnings. But, since these processors are so simple, maybe it would be best for me to use the new API.

Thanks for reviewing.

@joewitt

This comment has been minimized.

Copy link
Contributor

commented May 8, 2015

Tim

  1. You did a great job. That you did it at all is highly appreciated.
  2. This is fine. Because it is a test dependency and we're not pulling in source nor would it be part of a convenience binary we'd put out the burden is extremely small. It is also apache licensed so woohoo. Now it could pull in deps that aren't legit. I didn't check. But it doesn't matter given my previous statements (for this case).
  3. I am not a mongo expert at all. Perhaps this is worth tossing out as a question to the dev mailing list and see if anyone has an opinion on it you can discuss with.

We will do a more thorough review as well and send feedback.

But clearly a quality effort so thank you very much.
Joe

@brianghig

This comment has been minimized.

Copy link
Contributor

commented May 8, 2015

+1 for the 3.0 drivers. APIs are different, but not wildly. Exception / error handling seemed to be the biggest change (moving to generic MongoCommandExceptions with error codes and messages).

I've also done some work to support batch inserts of FlowFiles that we can merge into this to get some great throughput enhancements.

@tequalsme

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2015

No problem, however I think I made a last minute change that broke a test. I'll put up a new pull request.

@tequalsme

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2015

@brianghig will do on the 3.0 driver.

+1 on the batching. I thought it best to keep it simple initially.

@brianghig

This comment has been minimized.

Copy link
Contributor

commented May 8, 2015

@tequalsme Absolutely! This is a great start. I'm excited that's it here now, and looking forward to seeing how far we can take it.

@tequalsme

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2015

OK, I've updated the PR to use version 3.0.1 of the MongoDB Java driver.

@bjames301

This comment has been minimized.

Copy link

commented May 14, 2015

This is really cool Tim! Outstanding work!

@bjames301

This comment has been minimized.

Copy link

commented May 14, 2015

Tim, would it be helpful if I did some performance benchmarks using this processor? Writes to MongoDB tend to be very fast. This could help developers make informative choices on which processors they chose to use.

@markap14

This comment has been minimized.

Copy link
Contributor

commented May 15, 2015

Tim,

Only thing that I would change about the code is to make sure we do a Provenance SEND event for PutMongo. Otherwise, all looks great!

@tequalsme

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2015

Updated PR, adding a Provenance SEND event to PutMongo, and rebasing origin/develop.

@asfgit asfgit merged commit add03e3 into apache:develop May 19, 2015

@markap14

This comment has been minimized.

Copy link
Contributor

commented May 19, 2015

Nice work. Merged to develop.

Thanks for contributing this back!

@tequalsme tequalsme deleted the tequalsme:NIFI-570 branch Jun 1, 2015

tequalsme added a commit to tequalsme/nifi that referenced this pull request May 9, 2017

Merge pull request apache#53 from Asymmetrik/A-3699
[A-3699] Create PutCloudwatchMetric processor

JPercivall added a commit to JPercivall/nifi that referenced this pull request Apr 23, 2018

MINIFI-134 Removing test scope in the root pom from commons-io
This closes apache#53.

Signed-off-by: Aldrin Piri <aldrin@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.