cameronlee314 commented on a change in pull request #1001: SAMZA-2168: Remove
redundant SystemAdmin creation in ApplicationMaster
URL: https://github.com/apache/samza/pull/1001#discussion_r279138935
##########
File path:
samza-core/src/main/scala/org/apache/samza/job/local/ProcessJobFactory.scala
##########
@@ -52,54 +53,62 @@ class ProcessJobFactory extends StreamJobFactory with
Logging {
val configFromCoordinatorStream: Config =
CoordinatorStreamUtil.readConfigFromCoordinatorStream(coordinatorStreamStore)
- val changelogStreamManager = new ChangelogStreamManager(new
NamespaceAwareCoordinatorStreamStore(coordinatorStreamStore,
SetChangelogMapping.TYPE))
+ val systemAdmins = new SystemAdmins(configFromCoordinatorStream)
- val coordinator = JobModelManager(configFromCoordinatorStream,
changelogStreamManager.readPartitionMapping(), coordinatorStreamStore,
metricsRegistry)
- val jobModel = coordinator.jobModel
+ try {
+ systemAdmins.start()
Review comment:
Should this move out of the try-catch? It looks like the pattern you are
using for lifecycle management of `systemAdmins` is a bit inconsistent across
classes.
In some classes, you put the `start` inside the try-finally which has the
`stop`, but in other classes you have the `start` outside. Could you please try
to make it consistent?
----------------------------------------------------------------
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:
[email protected]
With regards,
Apache Git Services
