Github user joewitt commented on the pull request:
https://github.com/apache/incubator-nifi/pull/70#issuecomment-119416855
Alan,
Thanks for contributing! We'll work with you to get this promptly merged.
Here are some findings of an initial review:
* Please run the build with 'mvn clean install -Pcontrib-check'. You'll
find there many formatting issues. It appears that many of the lines have had
extraneous newlines and tabs added to them. Please take a look and tweak until
the contrib check runs cleanly and that you still think the code looks good.
* There do not appear to be any unit tests for the processor itself. I do
see a couple unit tests for the converter class/logic which is good but it is
best to also have a test or two, particularly for something that seems as
easily tested as this one is. You can find lots of examples throughout the
codebase including in this kite bundle.
* The conversion routines for Long, Double, Float, etc.. you really should
consider adding regex checks before calling those methods. Since the Java
methods use exception handling for flow control the performance penalty can be
extremely severe compared to simply doing a regex check beforehand. In the
event that the data is clean you'll be good to go but when it isn't the impact
to the system as a whole can be dramatic. Given the nature of this sort of
processor that is probably something to tackle right away.
* This processor is probably a great candidate to use the 'Advanced
Documentation' feature. Users will need this to understand the schema/syntax
of the conversion configuration and examples would go a long way for that. You
can see more about this here
http://nifi.incubator.apache.org/docs/nifi-docs/html/developer-guide.html#advanced-documentation
and there are some examples in the existing standard processors to consider.
* There are a couple of copy/paste errors in the processor from the
CSV/Avro converter. Look for these "Failed to convert {}/{} records from CSV
to Avro" and "Failed to convert {}/{} records from CSV to Avro"
I realize this looks like a lot of stuff but it should be pretty easy to
address and is a good first step. If you have any questions on it just let us
know.
Thanks
Joe
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
