Skip to content

NIFI-6999 - Made changes to load flow.xml files using streams. Update…#4715

Closed
thenatog wants to merge 4 commits intoapache:mainfrom
thenatog:NIFI-6999-rebased
Closed

NIFI-6999 - Made changes to load flow.xml files using streams. Update…#4715
thenatog wants to merge 4 commits intoapache:mainfrom
thenatog:NIFI-6999-rebased

Conversation

@thenatog
Copy link
Contributor

@thenatog thenatog commented Dec 7, 2020

…d tests.

NIFI-6999 - Slight change to test to check for WARN message.

NIFI-6999 - Removed very large flow file and test that uses it. This test ran for about 2 minutes so was excessive to keep in. The other changed tests to handle streams proves the functionality. A large file can be used on the command line to manually test large flow files. Some other cleanup.

Thank you for submitting a contribution to Apache NiFi.

Please provide a short description of the PR here:

Description of PR

Enables X functionality; fixes bug NIFI-YYYY.

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 main)?

  • 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 JDK 8?
  • Have you verified that the full build is successful on 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 GitHub Actions CI for build issues and submit an update to your PR as soon as possible.

…d tests.

NIFI-6999 - Slight change to test to check for WARN message.

NIFI-6999 - Removed very large flow file and test that uses it. This test ran for about 2 minutes so was excessive to keep in. The other changed tests to handle streams proves the functionality. A large file can be used on the command line to manually test large flow files. Some other cleanup.
@thenatog
Copy link
Contributor Author

thenatog commented Dec 8, 2020

This can be tested with the following files:
nifi-toolkit/nifi-toolkit-encrypt-config/src/test/resources/bootstrap.conf
nifi-toolkit/nifi-toolkit-encrypt-config/src/test/resources/nifi_default.properties
and the attached flow.xml.gz file. large-flow.xml.gz

The attached file is 10mb gzipped, and ~1GB unzipped. It has a processor with an encrypted field repeated many times:
enc{2032416987A00D9FCD757528D7AE609D7E793CA5F956641DB53E14CDB9BFCD4037B73AC705CD3F5C1C1BDE18B8D7B281} whose value is "thisIsABadPassword"

The command to run is:
./encrypt-config.sh -n conf/nifi.properties -f conf/large-flow.xml.gz -b conf/bootstrap.conf -x -v -s aNewPassword

Copy link
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

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

The refactored implementation looks good. See individual comments for small improvements. As noted in some of the comments on the unit test, some amount of cleanup there would be helpful.

@thenatog
Copy link
Contributor Author

thenatog commented Dec 8, 2020

Updated PR to remove junk comments and improve readability etc. as per your recommendation.

Copy link
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

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

Thanks for making the adjustments, looks much cleaner. See comments for three additional minor notes.

public String flowXmlPath
public String outputFlowXmlPath
public static flowXmlPath
public static outputFlowXmlPath
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason these values are static?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These were changed to static to allow static access in static main() for calling loadFlowXml(flowXmlPath). I changed this slightly to take the path as a parameter to allow easier testing for the loadFlowXml method. Not sure how successful it was though.

Copy link
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

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

The last round of changes look good. See one additional comment on method parameter naming to avoid shadowing class variable.

@thenatog
Copy link
Contributor Author

@exceptionfactory Updated with latest request. Let me know if there's anything else.

@exceptionfactory
Copy link
Contributor

Thanks for making the changes @thenatog. Looks good!

@thenatog
Copy link
Contributor Author

Will merge.

@thenatog thenatog closed this in 1c361d4 Jan 21, 2021
driesva pushed a commit to driesva/nifi that referenced this pull request Mar 19, 2021
…d tests.

NIFI-6999 - Slight change to test to check for WARN message.

NIFI-6999 - Removed very large flow file and test that uses it. This test ran for about 2 minutes so was excessive to keep in. The other changed tests to handle streams proves the functionality. A large file can be used on the command line to manually test large flow files. Some other cleanup.

NIFI-6999 - Removed comments and altered the code a little bit for readability as per code review.

NIFI-6999 - Removed commented code

NIFI-6999 - Renamed variable and removed assert comment.

Signed-off-by: Nathan Gough <thenatog@gmail.com>

This closes apache#4715.
krisztina-zsihovszki pushed a commit to krisztina-zsihovszki/nifi that referenced this pull request Jun 28, 2022
…d tests.

NIFI-6999 - Slight change to test to check for WARN message.

NIFI-6999 - Removed very large flow file and test that uses it. This test ran for about 2 minutes so was excessive to keep in. The other changed tests to handle streams proves the functionality. A large file can be used on the command line to manually test large flow files. Some other cleanup.

NIFI-6999 - Removed comments and altered the code a little bit for readability as per code review.

NIFI-6999 - Removed commented code

NIFI-6999 - Renamed variable and removed assert comment.

Signed-off-by: Nathan Gough <thenatog@gmail.com>

This closes apache#4715.
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.

2 participants