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 integration revision #1122

Merged
merged 12 commits into from Oct 2, 2023
Merged

Conversation

QuanticPony
Copy link
Contributor

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

* Rewrite processor to make them more consistent
* Split processor in smaller funcions
* Add timestamp field name
* Allow expression language in connection string
@chrisdutz
Copy link
Contributor

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.

@chrisdutz
Copy link
Contributor

By the way ... would be cool, if you could sign up to the dev-list and start communicating with us there ;-)

Copy link
Contributor

@ottobackwards ottobackwards 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 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

@QuanticPony
Copy link
Contributor Author

Sorry for the long PR. I would normally split it, but though posting as early as possible would be best in this case.
Duplicated code removed. Unit tests added.
i added the read tags from a file as I have seen it in some production environments with other OPCUA processors. I think it could be easier to migrate to PLC4X if we have them. If you don't see it clear i can remove it

@QuanticPony
Copy link
Contributor Author

I'm done. Tell me if anything else is needed. Will have a look at it over the weekend

@chrisdutz chrisdutz merged commit 4156cc9 into apache:develop Oct 2, 2023
5 of 14 checks passed
@TurkerTunali
Copy link

So, should we try the NiFi integration now?

@chrisdutz
Copy link
Contributor

Well it should be part of the Release ... so feel free to try.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants