-
Notifications
You must be signed in to change notification settings - Fork 401
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 integration revision #1122
Conversation
* Rewrite processor to make them more consistent * Split processor in smaller funcions * Add timestamp field name * Allow expression language in connection string
Thanks for this ;-) Unfortunately I am probably not the one ideal to review NiFi related things, but I would trust you and your changes. So if I don't hear any objections till Monday, I'll merge this PR into develop before cutting the release branch. |
By the way ... would be cool, if you could sign up to the dev-list and start communicating with us there ;-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the work!
- There is a lot of duplication of methods between each of the processors. These should be moved so that we don't have multiple copies in each processor
- I would like to see more tests for the new evaluation enabled things
- I would like to see tests around the caching with evaluations
- having properties read from files is not a great pattern for nifi, and there are no tests for it
in short, please add tests for all the new things and things that are now effected by the permutations possible. Please de-duplicate the methods in the various processors.
In the future, smaller pr's would make it easier to review
Sorry for the long PR. I would normally split it, but though posting as early as possible would be best in this case. |
I'm done. Tell me if anything else is needed. Will have a look at it over the weekend |
So, should we try the NiFi integration now? |
Well it should be part of the Release ... so feel free to try. |
NiFi integration revision
As we aim for a new release I bring a revision of the NiFi integration with a couple of improvements:
[Bug]: plc4j-tools-connection-cache: broken connections remaing in the cache on timeout #900: this made the processors be able to auto-reconnect with small changes. And allowed to make:
Add FilePropertyAccessStrategy: now we can read the addresses from a file in JSON format. Same as 'Address Text' property but in a file. Expression Language can be used and validation for the tags also works.
Processors code revision: rewrote the onTrigger methods of the processors so they look more similar between them. To make them more maintainable.
Complete FlowFlile transfer: some FlowFiles did not transfer to failure correctly. Now the incoming FlowFile is sent to failure and one attribute is added with the localized message in case of any exception.
Timestamp field name: It was a fixed
ts
field/attribute we added. I have made a property that defines this field's name.Update Readme to reflect changes.
Closes #593
Closes #629