SAMZA-2706: Clean up specific handling of diagnostics-specific metrics reporter#1550
Merged
cameronlee314 merged 2 commits intoapache:masterfrom Nov 8, 2021
Merged
Conversation
Contributor
|
LGTM, I'll create a jira for writing an integ test that tests out the behavior of data being emitted into diag-stream. |
ehoner
pushed a commit
to ehoner/samza
that referenced
this pull request
Apr 11, 2023
…s reporter (apache#1550) 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 file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issues: Specific checks for the
diagnosticsreportermetrics reporter were making the code for setting up diagnostics and thediagnosticsreportermetrics reporter a bit harder to work with. For the newStaticResourceJobCoordinator, it made object wiring harder for diagnostics-related components. #1021 initially had a reason to have special logic for creating thediagnosticsreportermetrics reporter, but #1369 made that special logic unnecessary.Changes:
DiagnosticsUtil.buildDiagnosticsManagerto only return aDiagnosticsManagerinstead ofPair<DiagnosticsManager, MetricsSnapshotReporter>.MetricsReporterLoader.getMetricsReportersno longer excludes thediagnosticsreportermetrics reporter. It just creates all configured reporters.MetricsReporterLoader.getMetricsReportersreturns all reporters, any classes that use that method no longer need to inject thediagnosticsreporterexplicitly.Tests:
./gradlew buildAPI changes:
diagnosticsreportermetrics reporter now is given aprocessorIdof "ApplicationMaster" instead of "samza-container-ApplicationMaster".MetricsReporterLoader.getMetricsReporters, ifdiagnosticsreporteris inmetrics.reporters, thendiagnosticsreportermetrics reporter is always created.diagnosticsreportermetrics reporter may still be created even ifjob.diagnostics.enabledis set to false. Note that Samza will only automatically create the stream for thediagnosticsreportermetrics reporter if diagnostics is enabled (since theDiagnosticsManageralso uses that same stream), so if there is a case in whichdiagnosticsreportermetrics reporter is needed while diagnostics is disabled, then the stream for the reporter needs to be created through some other means.