> On Nov. 10, 2016, 10:57 a.m., Prateek Maheshwari wrote: > > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java, line > > 103 > > <https://reviews.apache.org/r/53297/diff/5/?file=1560180#file1560180line103> > > > > Don't understand this interface. What's a SchedulingProvider, and how > > is it different than a ScheduledExecutorService? > > > > schedulingProvider should be probably be stopped in stop() in this > > class and not in SamzaMonitorService since this class owns it. Either that, > > or move instantiation to SamzaMonitorService too. > > Shanthoosh Venkataraman wrote: > SchedulingProvider interface looks similar to that of ExecutorService. > Not sure of the value add this interface provides. I guess, this is not > required and we could just use ExeuctorService. Added a jira for it, since > it's not in the scope of this patch. We could re-visit it later.
Sounds good, thanks. > On Nov. 10, 2016, 10:57 a.m., Prateek Maheshwari wrote: > > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java, line > > 107 > > <https://reviews.apache.org/r/53297/diff/5/?file=1560180#file1560180line107> > > > > Should this be called during start()? Same for stop(). > > Shanthoosh Venkataraman wrote: > To accomplish this, both(SamzaRestService,SamzaMonitorService) of them > had to be coupled. Logically they are seperate entities. One responsible for > resources and other monitors. Currently MonitorService & RestService are not > coupled with each other. It would be useful keep them seperate(In the future, > we could have SamzaRestService and SamzaMonitorService deployed seperately > when we have too many(Monitors & Resources) in them). I would prefer not to > have this coupling. Thanks for the explanation, makes sense to separate if they're independent. - Prateek ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53297/#review155625 ----------------------------------------------------------- On Nov. 10, 2016, 4:22 p.m., Shanthoosh Venkataraman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53297/ > ----------------------------------------------------------- > > (Updated Nov. 10, 2016, 4:22 p.m.) > > > Review request for samza and Jake Maes. > > > Repository: samza > > > Description > ------- > > This patch aims at enabling users to define custom reporters to send metrics > from the monitors. Configurations required for the definition of the metrics > reporters follows the same convention as of the samza jobs. > > > Diffs > ----- > > docs/learn/documentation/versioned/rest/monitors.md > 46678bbe5fed99f767c3324dc9578ee1a64cec66 > samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java > PRE-CREATION > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > e0468ee89c89fd720834461771ebb36475475bcb > > samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala > f24beb1e099dd44b15b475e0a4a7f70560c6965e > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java > 2a3e83a24a5343bb53b93fc9d0a647c1b253714b > samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java > PRE-CREATION > > samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala > 8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca > > Diff: https://reviews.apache.org/r/53297/diff/ > > > Testing > ------- > > Unit tests are done to verify the intended functionality. > > > Thanks, > > Shanthoosh Venkataraman > >
