Skip to content

SAMZA-2533: Fix DiagnosticsUtil to use configured MetricsReporterFactory#1369

Merged
mynameborat merged 11 commits intoapache:masterfrom
abhishekshivanna:diagutil
Jun 11, 2020
Merged

SAMZA-2533: Fix DiagnosticsUtil to use configured MetricsReporterFactory#1369
mynameborat merged 11 commits intoapache:masterfrom
abhishekshivanna:diagutil

Conversation

@abhishekshivanna
Copy link
Contributor

@abhishekshivanna abhishekshivanna commented May 27, 2020

DiagnosticsUtil#buildDiagnosticsManager currently hard codes
the creation of MetricsSnapshotReporter.
This patch fixes this and creates the MetricsReporter
using the factory defined in configs.

Edit (06/01): This patch also changes DiagnosticsUtil#buildDiagnosticsManager to use two separate producers for the MetricsSnapshortReporter and DiagnotsticsManager.

DiagnosticsUtil#buildDiagnosticsManager currently hard codes
the creation of MetricsSnapshotReporter.
This patch fixes this and creates the MetricsReporter
using the factory defined in configs.
@abhishekshivanna
Copy link
Contributor Author

Please note: MetricsSnapshotReporter#getProducer will be removed once #1368 is merged.

@xiefan46
Copy link
Contributor

lgtm.

Copy link
Contributor

@PawasChhokra PawasChhokra left a comment

Choose a reason for hiding this comment

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

LGTM.

@rmatharu-zz
Copy link
Contributor

Perhaps adding a test in TestDiagnosticsManager would be prudent?

@abhishekshivanna
Copy link
Contributor Author

Perhaps adding a test in TestDiagnosticsManager would be prudent?

This patch doesn't change the DiagnoticsManager (hence nothing to change in TestDiagnosticsManager ??). Instead, I added a test to verify that the reporter created (using DiagnosticsUtil#buildDiagnosticsManager) comes from the factory in configs.
Let me know if this looks good.

@abhishekshivanna
Copy link
Contributor Author

Based on comments received on #1368. This patch removes exposing the producer in MetricsSnapshotReporter.
Rather, instantiates two separate producers for DiagnotsticsManager and MetricsSnapshotReporter

Abhishek Shivanna added 2 commits June 9, 2020 16:45
Copy link
Contributor

@rmatharu-zz rmatharu-zz left a comment

Choose a reason for hiding this comment

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

fix and ship

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants