mynameborat commented on pull request #1518: URL: https://github.com/apache/samza/pull/1518#issuecomment-901354699
> > I remember something about the `MetricsSnapshotReporter` being instantiated within the `SamzaContainer` as well. Any plans to inject that as well from the higher layers e.g., `ContainerLaunchUtil` so that other components within can reuse the reporters? > > I think the change itself was not huge from the perspective of YARN although it had few considerations on the standalone front due to difference in lifecycle between `SamzaContainer` & `StreamProcessor` > > IMO, the way that Samza handles metrics in general is not very clean right now, and the refactoring needed to really clean it up is a bigger scope. There isn't very good dependency injection, so there isn't a single spot where metrics all get hooked up, so different components do their own reporter registration. Also, the reporters need to get started in some deep layer (e.g. `ContainerProcessManager` or `SamzaContainer.run`) because the registries need to be registered before starting the reporters (`MetricsReporter` doesn't define the contract about the ordering of registers/start, and at least I see one `MetricsReporter` implementation that assumes everything is registered first). > Therefore, I was just trying to do a more minimal change for now to get what I needed. Injecting the full set of reporters into `SamzaContainer` would clean up the code, but wouldn't add much more capability because `SamzaContainer` already registers all of the reporters with the registry that is now passed in. > Do you think this change itself is reasonable to put it on its own or do you think we should include some more clean up with it? Fair enough. Appreciate the context and background. I recollect some of these issues during my testing to refactor metrics reporter DI. Given you have deeper, fresh context on this, can you please collect all your context and ideas on this and create a ticket when possible. As far as this PR, I think it is fine to get this checked in. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
