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-6925: Fixed JoltTransformRecord for RecordReaders, improved MockProcessSession #3913

Merged
merged 4 commits into from Dec 17, 2019

Conversation

mattyb149
Copy link
Contributor

Thank you for submitting a contribution to Apache NiFi.

Please provide a short description of the PR here:

Description of PR

For RecordReader implementations that don't close the underlying input stream when nextRecord() returns no records, JoltTransformRecord fails the flowfile instead of sending the input flowfile to the "original" relationship. Unit tests didn't catch it because MockProcessSession wasn't keeping track of the open stream counts.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

  • Is there a JIRA ticket associated with this PR? Is it referenced
    in the commit message?

  • 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.

  • Has your PR been rebased against the latest commit within the target branch (typically master)?

  • Is your initial contribution a single, squashed commit? Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not squash or use --force when pushing to allow for clean monitoring of changes.

For code changes:

  • 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?
  • Have you verified that the full build is successful on both JDK 8 and JDK 11?
  • 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.

@mattyb149
Copy link
Contributor Author

I believe the change to MockProcessSession has found a bug in PutElasticsearchHttpRecord, as TestPutElasticsearchHttpRecord is failing. I'll take a look at that processor and update it as necessary. Hopefully the change is for the better and will find such bugs that unit tests haven't previously detected.

} catch (final Exception ex) {
logger.error("Unable to transform {} due to {}", new Object[]{original, ex.toString(), ex});
session.transfer(original, REL_FAILURE);
session.remove(transformed);
Copy link
Contributor

Choose a reason for hiding this comment

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

Recommend adding a null check here. In testing I saw a case where if an exception is encountered the transformed object is null at this point. When session.remove is called on the object it hits a NullPointer exception.

transformed = session.putAllAttributes(transformed, attributes);
logger.info("{} had no Records to transform", new Object[]{original});
} else {

final JoltTransform transform = getTransform(context, original);
final Record transformedFirstRecord = transform(firstRecord, transform);
Copy link
Contributor

Choose a reason for hiding this comment

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

If the transform fails here transformedFirstRecord is returned and causes a NullPointerException on the next line for writeSchema. Recommend adding a null check.

@YolandaMDavis
Copy link
Contributor

Was able to configure a setup to validate in Hive. Appreciate your patience (and help) @mattyb149 . Also fixes for elasticsearch were confirmed as well

+1

Will merge shortly

@YolandaMDavis YolandaMDavis merged commit 6d1bcc2 into apache:master Dec 17, 2019
patricker pushed a commit to patricker/nifi that referenced this pull request Jan 22, 2020
…ProcessSession (apache#3913)

* NIFI-6925: Fixed JoltTransformRecord for RecordReaders, improved MockProcessSession

* Fixed logic for no records, added unit test

* Fixed PutElasticsearchHttpRecord and PutHive3Streaming, same bug as JoltTransformRecord

* Added null checks
natural pushed a commit to natural/nifi that referenced this pull request Feb 1, 2020
…ProcessSession (apache#3913)

* NIFI-6925: Fixed JoltTransformRecord for RecordReaders, improved MockProcessSession

* Fixed logic for no records, added unit test

* Fixed PutElasticsearchHttpRecord and PutHive3Streaming, same bug as JoltTransformRecord

* Added null checks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants