Skip to content

Conversation

@mattyb149
Copy link
Contributor

No description provided.

@mattyb149 mattyb149 force-pushed the script-processors branch 2 times, most recently from 44f1dcc to 0d86989 Compare January 25, 2016 21:58
Copy link
Contributor

Choose a reason for hiding this comment

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

This processor should be annotated with @InputRequirement(Requirement.INPUT_REQUIRED) since it can't be executed without an input FlowFile.

Copy link
Contributor

Choose a reason for hiding this comment

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

since we're giving the script access to the session and since we cannot possible know its intent I say we stop pulling a flowfile for them and let them do it. In so doing we can remove the input required need as well and let scripts be used as ways to source data as well. Basically "Here script - we've given you a process session - have fun but you're responsible for accounting for the flow files transfers"

Copy link
Contributor

Choose a reason for hiding this comment

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

absolutely agree with @joewitt. Removing the requirement for input opens up a lot more use cases.

@markap14
Copy link
Contributor

@mattyb149 wow this set of Processors is quite a feat! :) I provided feedback in the way of several inline comments. The only other thing that I would recommend is to consider factoring out a parent Abstract class or a util class. It seems like both Processors have quite a lot of overlap that could be simplified with either one of those approaches. Very cool to get these Processors in though! THanks for all of the work that you've put into getting this done!

@mattyb149
Copy link
Contributor Author

TL;DR Will take a fresh look at this :)

I wanted very badly to have a parent Abstract base class, but the two processors extend from different parent classes (AbstractProcessor and AbstractSessionFactoryProcessor). Now that the behavior of ExecuteScript has changed to let the script handle the session and transfers, perhaps I can refactor that, at which point I will certainly pull the common code out. I thought about a Util class too but the methods looked like they'd need large signatures to get all the dependencies injected.

@mattyb149 mattyb149 closed this Jan 26, 2016
@mattyb149 mattyb149 reopened this Jan 26, 2016
Copy link
Contributor

Choose a reason for hiding this comment

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

You added a comment indicating that custom validation will ensure that exactly 1 of the Script File or Script Body is populated but the implementation just delegates to the super class, which returns an empty collection, I believe..

@mattyb149 mattyb149 changed the title NIFI-210: Add ExecuteScript and InvokeScriptProcessor NIFI-210: Add ExecuteScript and InvokeScriptedProcessor Jan 30, 2016
@mattyb149 mattyb149 force-pushed the script-processors branch 4 times, most recently from 4d6afff to ff86c68 Compare February 1, 2016 21:18
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like something a bit wonky is happening here. So if one enters a valid script then enters a buggy script the original value is set and not being replaced. Then because if there is an invalid script and no modulepath the error is being buried. Definitely need the old script/processor that was compiled to get wiped out and we need the errors to always make it to the log/bulletin.

@asfgit asfgit closed this in 77ab5d3 Feb 2, 2016
mattyb149 pushed a commit to mattyb149/nifi that referenced this pull request Dec 9, 2020
…ndle all subsequent versions

Add test for supplied sample file

This closes apache#188 and closes apache#185.

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