Skip to content

Conversation

@PuspenduBanerjee
Copy link
Contributor

It's initial working version of nifi-camel integration.
It Supports to load camel context from File, Classpath as well as to define and modify Routes on the fly.
Overall it's an implementation of feature request NIFI-924 [https://issues.apache.org/jira/browse/NIFI-924#]

@PuspenduBanerjee PuspenduBanerjee changed the title Nifi-Camel Integration Initial Commit Nifi-Camel Integration Jan 24, 2016
@PuspenduBanerjee
Copy link
Contributor Author

Any plan to pull this PR?

@joewitt
Copy link
Contributor

joewitt commented Jan 25, 2016

Hello

The community is working toward closing down on the 050 release so PR review bandwidth is limited right now. This PR represents a pretty significant step towards integration with another framework so we need to be thoughtful about the user experience. So while this represents a very interesting and compelling integration it may take a bit of time to get right.

Now the PR as-is there are a few quick items I've noticed.

  • It still appears the license and notice information included is copy/paste from the Flume nar as originally identified. Those are very important to to get right.
  • There does not appear to be any tests.
  • Documentation on the approach, idea, and processor is very limited. There is a sentence about it on the JIRA but it would be really helpful to understand more about the idea and how it compliments NiFi and Camel. Users and developers will definitely want to know this.
  • There is a declaration of ThreadLocal usage and then the use of it appears commented out. This will need to be removed.
  • There is use of e.printStackStrack() and this needs to be using the provided processor Logger.
  • A camel application context is provided in src/resources but it appears it should be a test resource

Very much appreciate the contribution and effort. Just need your understanding that things this important take time and a lot of diligence.

Thanks
Joe

@PuspenduBanerjee
Copy link
Contributor Author

Hello Joe,
Thanks for the review.

About Licensing, I am trying to add some maven plugin to automate that process and strongly believe that it should be centralized [ as rat plugin is checking License ].
Any help in this regard will be very much appreciated.

About documentation, will it be a good idea to add a README file or do you have any other suggestion?

And yes , there are some WIP code containing e.printStackStrack() , ThreadLocal etc. Working to get those removed.

Thanks again.
Puspendu Banerjee

@joewitt
Copy link
Contributor

joewitt commented Jan 26, 2016

Ok understood. So given this initial feedback it sounds like you're still working on the PR and now have some things to work for it. When you are ready to submit again can you make sure the JIRA or the PR itself has a template and sample camel context that could be used to quickly get up to speed and try it out?

@PuspenduBanerjee
Copy link
Contributor Author

Hello,
Now my build is failing in Travis CI, where as it's running fine locally.
Can anyone please help

[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 01:07 min
[INFO] Finished at: 2016-01-27T06:50:28+00:00
[INFO] Final Memory: 150M/420M
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-remote-resources-plugin:1.5:process (default) on project nifi-kite-processors: Failed to resolve dependencies for one or more projects in the reactor. Reason: Missing:
[ERROR] ----------
[ERROR] 1) org.apache.hive:hive-serde:jar:0.12.0-cdh5.0.0
[ERROR]
[ERROR] Try downloading the file manually from the project website.
[ERROR]
[ERROR] Then, install it using the command:
[ERROR] mvn install:install-file -DgroupId=org.apache.hive -DartifactId=hive-serde -Dversion=0.12.0-cdh5.0.0 -Dpackaging=jar -Dfile=/path/to/file
[ERROR]
[ERROR] Alternatively, if you host your own repository you can deploy the file there:
[ERROR] mvn deploy:deploy-file -DgroupId=org.apache.hive -DartifactId=hive-serde -Dversion=0.12.0-cdh5.0.0 -Dpackaging=jar -Dfile=/path/to/file -Durl=[url] -DrepositoryId=[id]
[ERROR]
[ERROR] Path to dependency:
[ERROR] 1) org.apache.nifi:nifi-kite-processors🫙0.4.2-SNAPSHOT
[ERROR] 2) org.apache.hive.hcatalog:hive-hcatalog-core:jar:1.2.0
[ERROR] 3) org.apache.hive:hive-metastore:jar:1.2.0

@apiri
Copy link
Member

apiri commented Jan 27, 2016

@PuspenduBanerjee This was a commit that had reliance on repositories outside of those as part of the core build and has been reverted, more details are available in #147. Please pull/rebase the latest changes from master and you should be good to go.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to eliminate extra dependency, it is preferable to rely on native java classes if they provide such functionality and in this case they do for both List and Set (see Collections.unmodifiable. . .)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am seeing that other components are using the same and as we already have dependency on google-guava, I guess it should be okay. Please correct me otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True and that is a potential for improvement. All I am saying is that this module brings a dependency that may have also some transient dependency for something that is not very well utilized. You are only using 2 classes from it and those two classes have adequate equivalents in JDK. Always think about the distribution of a NAR and its size. Rule of thumb, the thinner the better.

@PuspenduBanerjee
Copy link
Contributor Author

@apiri Thanks. It has built correctly with intermittent Heap issue. 👍

@PuspenduBanerjee
Copy link
Contributor Author

@apiri Again OOM in travis:

testExecuteIngestAndUpdate(org.apache.nifi.processors.standard.TestExecuteStreamCommand) Time elapsed: 1.717 sec <<< ERROR!
java.lang.OutOfMemoryError: Java heap space
at java.lang.StringCoding$StringDecoder.decode(StringCoding.java:149)
at java.lang.StringCoding.decode(StringCoding.java:193)
at java.lang.StringCoding.decode(StringCoding.java:254)
at java.lang.String.(String.java:536)
at java.lang.String.(String.java:556)
at org.apache.nifi.processors.standard.TestExecuteStreamCommand.testExecuteIngestAndUpdate(TestExecuteStreamCommand.java:128)

@PuspenduBanerjee
Copy link
Contributor Author

Hello @joewitt

PFA sample camel contexts.
sample-application-contexts.zip

CamelProcessorTestingTemplate.xml.zip

  • You can copy the content & directly paste at *_Camel Spring Context Definition *_property.
  • In case you require some extra camel module add their maven groupId:artifactId:version in groupId/artifactId/version form [GAV]. Multiple GAVs can be separated by ',' at *_Extra Libraries *_property..

I shall attach a presentation or some other visual representation, as we progress with the PR.

Thanks.

@PuspenduBanerjee
Copy link
Contributor Author

Closing PR, as found some issue in Integration test. Shall raise a new PR later.

@PuspenduBanerjee
Copy link
Contributor Author

Moved to : PR #197

@PuspenduBanerjee
Copy link
Contributor Author

@joewitt Seeing that Travis build has started to skip tests. Is that a intended decision?
I am asking as I am not seeing such configuration in .travis.yml.

@joewitt
Copy link
Contributor

joewitt commented Feb 2, 2016

definitely not intended. Can you send me a link? We've seen much better
results with it now. There have been failures but they have been correct
failures.

On Tue, Feb 2, 2016 at 10:13 AM, Puspendu Banerjee <notifications@github.com

wrote:

@joewitt https://github.com/joewitt Seeing that Travis build has
started to skip tests. Is that a intended decision?
I am asking as I am not seeing such configuration in .travis.yml.


Reply to this email directly or view it on GitHub
#186 (comment).

@PuspenduBanerjee
Copy link
Contributor Author

@joewitt Please find mvn install -DskipTests=true -Dmaven.javadoc.skip=true -B -V near line 170 in all build at my PR

@apiri
Copy link
Member

apiri commented Feb 2, 2016

@PuspenduBanerjee Certainly a bit confusing, but this is the standard sequence that Travis employs when building Maven based projects. This is for accumulating all dependencies up front before performing the actual build. Don't have the line numbers handy, but if you perform a search for:

mvn clean install -Pcontrib-check,groovy-unit-test

You can see where the official and full build takes place. Hope this clears things up!

@PuspenduBanerjee
Copy link
Contributor Author

@apiri Still have doubt.
A push fires a PR build .If that build doesn't run Test Cases , things can become scary after merging with main line.

@apiri
Copy link
Member

apiri commented Feb 2, 2016

@PuspenduBanerjee Not sure I understand. From the linked PR in Travis I looked at one of the build logs (https://s3.amazonaws.com/archive.travis-ci.org/jobs/106415261/log.txt) and found the following among all the other typical test cases:


T E S T S

Running org.apache.nifi.processors.camel.CamelProcessorTest
Tests run: 7, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 3.956 sec - in org.apache.nifi.processors.camel.CamelProcessorTest

Do you have any additional details as to which tests you are seeing get skipped? To clarify, Travis maven builds are two steps:

  1. Gather all dependencies for the build and compile: mvn install -DskipTests=true -Dmaven.javadoc.skip=true -B -V
  2. Do a full install and contrib-check: mvn clean install -Pcontrib-check,groovy-unit-test

Thanks!

@PuspenduBanerjee
Copy link
Contributor Author

@apiri Sorry! It's my bad. I looked into a wrong location.

@apiri
Copy link
Member

apiri commented Feb 2, 2016

No worries! Just wanted to make sure we weren't missing anything.

mattyb149 pushed a commit to mattyb149/nifi that referenced this pull request Dec 9, 2020
…n Guide

This closes apache#186.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants