----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53297/#review155625 -----------------------------------------------------------
Looks pretty good to me. Some final minor comments and questions. docs/learn/documentation/versioned/rest/monitors.md (line 98) <https://reviews.apache.org/r/53297/#comment225624> Maybe: "are the same as that of Samza jobs." and make "that of Samza jobs" the configuration link and remove next line? samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java (line 30) <https://reviews.apache.org/r/53297/#comment225627> Remove "contains reusable methods which" and "based upon configuration". Don't mind removing the whole comment either. samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java (line 39) <https://reviews.apache.org/r/53297/#comment225628> I meant remove the whole javadoc including @params. There's no useful additional information here. samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java (line 52) <https://reviews.apache.org/r/53297/#comment225632> Minor, your call: Not a big fan of the "hanging towers of parameters" code style. Maybe extract variable? samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala (line 45) <https://reviews.apache.org/r/53297/#comment225629> Maybe use JavaCoverters._ and .asScala? samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java (line 27) <https://reviews.apache.org/r/53297/#comment225630> Unused import. samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java (line 91) <https://reviews.apache.org/r/53297/#comment225631> See previous comment about just passing the entire config. Does that not work? samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java (line 103) <https://reviews.apache.org/r/53297/#comment225638> 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. samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java (line 107) <https://reviews.apache.org/r/53297/#comment225636> Should this be called during start()? Same for stop(). samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java (line 164) <https://reviews.apache.org/r/53297/#comment225633> debug? samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java (line 166) <https://reviews.apache.org/r/53297/#comment225634> debug? samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala (line 46) <https://reviews.apache.org/r/53297/#comment225641> Maybe use JavaCoverters._ and .asScala? - Prateek Maheshwari On Nov. 9, 2016, 5:04 p.m., Shanthoosh Venkataraman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53297/ > ----------------------------------------------------------- > > (Updated Nov. 9, 2016, 5:04 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/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 > >
