[GitHub] [samza] mynameborat commented on pull request #1368: SAMZA-2532: Refactor MetricsSnapshotReporter and MetricsSnapshotReporterFactory

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

mynameborat commented on pull request #1368:
URL: https://github.com/apache/samza/pull/1368#issuecomment-636024955


   Changes w.r.t exception and refactoring run looks good to me. I have few questions on why expose the `getProducer` within the `MetricsSnapshotReporter`.
   
   > so we can share the existing producer with
   when creating wrappers around the MetricsSnapshotReporter's stream (eg: DiagnosticsManager)
   
   1. Can you explain why you need to share producers here?
   2. By wrappers, do you mean class extensions of MetricsSnapshotReporter? Ideally we should inject the shared objects during the construction and manage the lifecycle of the shared object outside the scope of its dependencies. With the current setup, `MetricsSnapshotReporter` manages the lifecycle of the producer & may choose to handle lifecycle of the `producer` that doesn't necessarily align with the expectation of `DiagnosticsManager` and cause problems. e.g. `MetricSnapshotReporter` may choose to close the producer or recreate producer because of a failure and `DiagnosticsManager` can perform operations on the handle of producer that is no longer active or that is stale.


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

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