nifi icon indicating copy to clipboard operation
nifi copied to clipboard

NIFI-10287 ExecuteScript - Allow python scripts to use external modules

Open NissimShiman opened this issue 3 years ago • 4 comments

Summary

NIFI-10287 This resolves a bug that crept in where the ExecuteScript processor used to support using external modules for python, but has lost that functionality. This has been fixed in this ticket.

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

Pull Request Tracking

  • [x] Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
  • [x] Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

  • [x] Pull Request based on current revision of the main branch
  • [x] Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Build

  • [x] Build completed using mvn clean install -P contrib-check
    • [x] JDK 8
    • [x] JDK 11
    • [x] JDK 17

Licensing

  • [x] New dependencies are compatible with the Apache License 2.0 according to the License Policy
  • [x] New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

  • [x] Documentation formatting appears as expected in rendered files

NissimShiman avatar Jul 28 '22 15:07 NissimShiman

LGTM+1

dan-s1 avatar Sep 12 '22 16:09 dan-s1

Thank you very much @dan-s1 for the review and for volunteering the unit test for the use case that was missed!

Squashed commits after approval to make it easier for additional reviewers to see code and to capture attribution for @dan-s1's generous contribution.

NissimShiman avatar Sep 12 '22 19:09 NissimShiman

Unit test looks good. I built the code (including -Pcontrib-check), and all looks good. I tested by instantiating an ExecuteScript processor. It was configured with a module directory with a .py module file similar to the one used in the unit test. Ran data and it worked like a charm. I changed the module code and re-ran a flowfile (without stopping the processor.) The changes were not reflected in the processed flowfile. However, restarting the processor picks up the module update. I believe this is expected behavior since we do not want to reload modules (or even check if they should be reloaded) on every flowfile; I believe that would be overly burdensome.

Overall, looks good to merge to me. Thanks @NissimShiman ! +1

markobean avatar Sep 23 '22 17:09 markobean

@mattyb149 @exceptionfactory @MikeThomsen I removed the unit test that was concerning so I think we are good. The unit test for the main use case of the ticket remains. Please let me know if there are other holdup related concerns.

NissimShiman avatar Oct 03 '22 15:10 NissimShiman

@mattyb149 I was wondering if you could take a look at this again. The previous issue you (and another reviewer) mentioned has been resolved.

NissimShiman avatar Oct 20 '22 14:10 NissimShiman

+1 LGTM, thanks for the improvement and sticking with this! Merging to main

mattyb149 avatar Nov 02 '22 20:11 mattyb149

Thank you @mattyb149 !

NissimShiman avatar Nov 03 '22 00:11 NissimShiman