mattyb149 commented on a change in pull request #3898: NIFI-6886 - Bugfix
site-to-site-reporting-task
URL: https://github.com/apache/nifi/pull/3898#discussion_r363402405
##########
File path:
nifi-nar-bundles/nifi-site-to-site-reporting-bundle/nifi-site-to-site-reporting-task/src/main/java/org/apache/nifi/reporting/AbstractSiteToSiteReportingTask.java
##########
@@ -118,9 +116,10 @@
return properties;
}
- @OnScheduled
- public void setup(final ConfigurationContext context) throws IOException {
- siteToSiteClient = SiteToSiteUtils.getClient(context, getLogger());
+ public void setup(final ReportingContext reportContext) throws IOException
{
+ if (siteToSiteClient != null) {
Review comment:
I believe this is a negative-logic error, as the real `siteToSiteClient`
never gets set. If I run this with a `SiteToSiteProvenanceReportingTask`, then
as soon as a provenance event is generated and the reporting task runs, it
fails with a NullPointerException.
The unit tests did not catch it because the
`MockSiteToSiteProvenanceReportingTask.getClient()` method does not use this
one. For consistency of behavior, I'm thinking with the new `setup()` method in
`AbstractSiteToSiteReportingTask` (as well as the `getClient()` method for
testing, `MockSiteToSiteProvenanceReportingTask` should not override
`getClient()` but instead override `setup()` and create the mock there. That
still won't catch the above logic error, but that will be hard to do with a
unit test anyway as that method is trying to create a real client. We could
include the same logic in the mocked `setup()` method though, to check if the
client is null and create it if it is.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
