> On Nov. 9, 2016, 12:09 a.m., Jagadish Venkatraman wrote: > > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java, line > > 65 > > <https://reviews.apache.org/r/53297/diff/2/?file=1556965#file1556965line65> > > > > Why is this taking in a concrete class - ReadableMetricsRegistry ? > > Prefer to have this `MetricsRegistry` instead. > > > > We should operate at the level of interfaces whereever possible.
ReadableMetricsRegistry is an interface, not a concrete class. This particular type is required for registering with MetricsReporter instances. > On Nov. 9, 2016, 12:09 a.m., Jagadish Venkatraman wrote: > > docs/learn/documentation/versioned/rest/monitors.md, line 95 > > <https://reviews.apache.org/r/53297/diff/2/?file=1556960#file1556960line95> > > > > Documentation looks different (than the rest of the page). Would be > > nice if this was instructional in nature. > > > > /s/create/create and report. > > /s/theirs/yours Done. > On Nov. 9, 2016, 12:09 a.m., Jagadish Venkatraman wrote: > > docs/learn/documentation/versioned/rest/monitors.md, line 99 > > <https://reviews.apache.org/r/53297/diff/2/?file=1556960#file1556960line99> > > > > 1. s/Please refer/You can refer > > 2. `custom` is probably redundant. > > 3. The link and anchor text placement is weird. > > 4. Prefer to be precise with documentation. (For example, this line has > > the word `metrics reporters` thrice) Done. > On Nov. 9, 2016, 12:09 a.m., Jagadish Venkatraman wrote: > > docs/learn/documentation/versioned/rest/monitors.md, line 100 > > <https://reviews.apache.org/r/53297/diff/2/?file=1556960#file1556960line100> > > > > This line is not needed IMO. (If we choose to retain, it, should > > atleast be re-worded) Removed. > On Nov. 9, 2016, 12:09 a.m., Jagadish Venkatraman 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> > > > > How exactly is this used when metrics are reported? > > > > Is there a reason for this to be configurable / adding a new config > > key? > > > > If it's configurable, we should document it somewhere? 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. - Shanthoosh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53297/#review155372 ----------------------------------------------------------- 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 > >
