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-4809 - Implement a SiteToSiteMetricsReportingTask #2575

Closed
wants to merge 3 commits into from

Conversation

pvillard31
Copy link
Contributor

This PR is an update of the previous one (#2430) that has been reverted to fix a dependency issue.

I've manually added a JSON Record reader in the abstract class using the code of the existing reader. For now, it's only used by the Metrics reporting task, but I plan to use the same approach for the other S2S reporting tasks.

Do not merge this PR as-is.

I currently see two issues:

  • the one already reported by @mattyb149: the record writer is not showing the reporting task in the "referencing components". I think it's probably a framework issue that could be solved in a separate JIRA. Will try to have a look later.
  • more importantly, I see an issue when using a writer with an access schema strategy using the Avro Schema registry CS. In that case, I get a Schema not found exception "Failed to write metrics using record writer: Unable to find schema with name 'metrics'". Not sure what is causing it, will have a look asap. If using "inherit record schema" strategy, it works as expected.

Wanted to open this PR, to have feedbacks on the approach regarding the JSON record reader.

@mattyb149
Copy link
Contributor

I saw the "unable to find schema" issue in the last PR too, it seems to be related to the onPropertyModified() method of AvroSchemaRegistry, that's where the actual schemas get populated, but while debugging I saw that the name was in the schema map but there was no actual schema stored. Not sure if this is a lifecycle order-of-operations thing or not, but when I deleted the schema from the registry, saved it, then added it back, it worked. Just some extra info while you're pathfinding :)

@pvillard31
Copy link
Contributor Author

Both issues seem to be resolved on master, I have rebased against master.

@mattyb149
Copy link
Contributor

Mind doing another rebase? I will review shortly thereafter, thanks!

<dependency>
<groupId>org.apache.nifi</groupId>
<artifactId>nifi-reporting-utils</artifactId>
<version>1.6.0-SNAPSHOT</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

You'll need to update these after rebase

@pvillard31
Copy link
Contributor Author

Done, thanks @mattyb149 !

@mattyb149
Copy link
Contributor

+1 LGTM, ran full build with unit tests and tried Ambari Format as well as Record Format with an AvroRecordSetWriter. Verified the records are in the prescribed format and the standard S2S reporting task attributes are correct. Also the documentation is great, thanks for this addition! Merging to master

@asfgit asfgit closed this in 6fbe151 Apr 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants