> On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote: > > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java, line 27 > > <https://reviews.apache.org/r/53297/diff/5/?file=1560179#file1560179line27> > > > > Unused import.
Removed. > On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote: > > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java, line > > 164 > > <https://reviews.apache.org/r/53297/diff/5/?file=1560180#file1560180line164> > > > > debug? I think this should be info. It will be useful to know the registered reporters while debugging. > On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote: > > samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java, > > line 39 > > <https://reviews.apache.org/r/53297/diff/5/?file=1560176#file1560176line39> > > > > I meant remove the whole javadoc including @params. There's no useful > > additional information here. Removed. > On Nov. 10, 2016, 6:57 p.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(). 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. > On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote: > > samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala, > > line 56 > > <https://reviews.apache.org/r/53297/diff/5/?file=1560182#file1560182line56> > > > > Maybe use JavaCoverters._ and .asScala? Done. > On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote: > > samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala, > > line 46 > > <https://reviews.apache.org/r/53297/diff/5/?file=1560178#file1560178line46> > > > > Maybe use JavaCoverters._ and .asScala? Done. > On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote: > > samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java, > > line 30 > > <https://reviews.apache.org/r/53297/diff/5/?file=1560176#file1560176line30> > > > > Remove "contains reusable methods which" and "based upon > > configuration". Don't mind removing the whole comment either. Removed. > On Nov. 10, 2016, 6:57 p.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. 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. > On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote: > > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java, line > > 166 > > <https://reviews.apache.org/r/53297/diff/5/?file=1560180#file1560180line166> > > > > debug? I think this should be info. Will be useful to know this when debugging. > On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote: > > docs/learn/documentation/versioned/rest/monitors.md, line 98 > > <https://reviews.apache.org/r/53297/diff/5/?file=1560175#file1560175line98> > > > > Maybe: "are the same as that of Samza jobs." and make "that of Samza > > jobs" the configuration link and remove next line? Done. > On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote: > > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java, line > > 91 > > <https://reviews.apache.org/r/53297/diff/5/?file=1560180#file1560180line91> > > > > See previous comment about just passing the entire config. Does that > > not work? Done. - Shanthoosh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53297/#review155625 ----------------------------------------------------------- On Nov. 10, 2016, 1:04 a.m., Shanthoosh Venkataraman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53297/ > ----------------------------------------------------------- > > (Updated Nov. 10, 2016, 1:04 a.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/SamzaRestConfig.java > 47b0663637f6db187d86961377ee3ee203b73fdb > 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 > >
