> On Nov. 9, 2016, 12:31 a.m., Prateek Maheshwari wrote: > > samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java, > > line 36 > > <https://reviews.apache.org/r/53297/diff/2/?file=1556961#file1556961line36> > > > > Minor: private constructors for helper classes are pretty universal, > > don't think they need a comment.
Done. > On Nov. 9, 2016, 12:31 a.m., Prateek Maheshwari wrote: > > docs/learn/documentation/versioned/rest/monitors.md, line 100 > > <https://reviews.apache.org/r/53297/diff/2/?file=1556960#file1556960line100> > > > > This section could be more concise. Changed, removed some of the docs. > On Nov. 9, 2016, 12:31 a.m., Prateek Maheshwari wrote: > > samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java, > > line 40 > > <https://reviews.apache.org/r/53297/diff/2/?file=1556961#file1556961line40> > > > > This javadoc doesn't add any more information than the method signature > > already provides. Prefer removing. Removed. > On Nov. 9, 2016, 12:31 a.m., Prateek Maheshwari wrote: > > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java, line 55 > > <https://reviews.apache.org/r/53297/diff/2/?file=1556964#file1556964line55> > > > > What is this config, and what does container refer to here? The > > samza-rest service container? Container name here refers to the execution unit from which the metrics gets reported. It's used when instantiating the MetricsReporter. If this configuration is undefined, it's defaulted to hostName on which SamzaRest is running. Resources will report metrics alongside monitors. Added relevent documentation for it. > On Nov. 9, 2016, 12:31 a.m., Prateek Maheshwari wrote: > > samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java, > > line 45 > > <https://reviews.apache.org/r/53297/diff/2/?file=1556961#file1556961line45> > > > > Not sure this specific method (for classloading this particular class > > from configs) deserves a new Util class. This is a general pattern used in > > other places, maybe we should extract that as a util instead. > > > > May be worth revisiting if/how we want to share code b/w samza-rest and > > samza-core. Ideally samza-rest shouldn't depend on samza-core, and shared > > stuff should be pulled out to samza-api or samza-common (which doesn't > > exist yet). Discussed offline. Extracting utils(and other common functionalities) shared between samza-core & samza-rest into samza-common is a longer term goal which is required to remove dependencies. However, for the scope of this patch, doing this will be a overkill. Punted for now. - Shanthoosh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53297/#review155380 ----------------------------------------------------------- On Nov. 9, 2016, 10:50 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, 10:50 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 > docs/learn/documentation/versioned/rest/overview.md > c382f032843cce696a445ff110e87a8198cc96d7 > 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 > >
