[GitHub] [samza] cameronlee314 opened a new pull request #1550: SAMZA-2706: Clean up specific handling of diagnostics-specific metrics reporter

[GitBox <gi...@apache.org>]

cameronlee314 opened a new pull request #1550:
URL: https://github.com/apache/samza/pull/1550


   Issues: Specific checks for the `diagnosticsreporter` metrics reporter were making the code for setting up diagnostics and the `diagnosticsreporter` metrics reporter a bit harder to work with. For the new `StaticResourceJobCoordinator`, it made object wiring harder for diagnostics-related components. https://github.com/apache/samza/pull/1021 initially had a reason to have special logic for creating the `diagnosticsreporter` metrics reporter, but https://github.com/apache/samza/pull/1369 made that special logic unnecessary.
   
   Changes:
   1. Changed `DiagnosticsUtil.buildDiagnosticsManager` to only return a `DiagnosticsManager` instead of `Pair<DiagnosticsManager, MetricsSnapshotReporter>`.
   2. `MetricsReporterLoader.getMetricsReporters` no longer excludes the `diagnosticsreporter` metrics reporter. It just creates all configured reporters.
   3. Since `MetricsReporterLoader.getMetricsReporters` returns all reporters, any classes that use that method no longer need to inject the `diagnosticsreporter` explicitly.
   
   Tests:
   `./gradlew build`
   
   API changes:
   1. In the YARN application master,`diagnosticsreporter` metrics reporter now is given a `processorId` of "ApplicationMaster" instead of "samza-container-ApplicationMaster".
   2. When using `MetricsReporterLoader.getMetricsReporters`, if `diagnosticsreporter` is in `metrics.reporters`, then `diagnosticsreporter` metrics reporter is always created.
   3. The `diagnosticsreporter` metrics reporter may still be created even if `job.diagnostics.enabled` is set to false. Note that Samza will only automatically create the stream for the `diagnosticsreporter` metrics reporter if diagnostics is enabled (since the `DiagnosticsManager` also uses that same stream), so if there is a case in which `diagnosticsreporter` metrics reporter is needed while diagnostics is disabled, then the stream for the reporter needs to be created through some other means.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [samza] cameronlee314 merged pull request #1550: SAMZA-2706: Clean up specific handling of diagnostics-specific metrics reporter

[GitBox <gi...@apache.org>]

cameronlee314 merged pull request #1550:
URL: https://github.com/apache/samza/pull/1550


   


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [samza] rmatharu commented on pull request #1550: SAMZA-2706: Clean up specific handling of diagnostics-specific metrics reporter

[GitBox <gi...@apache.org>]

rmatharu commented on pull request #1550:
URL: https://github.com/apache/samza/pull/1550#issuecomment-962243787


   LGTM, 
   
   I'll create a jira for writing an integ test that tests out the behavior of data being emitted into diag-stream. 
   Currently we rely on rel-test for this.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org