> On Nov. 5, 2016, 6:47 p.m., Jake Maes wrote: > > samza-core/src/main/scala/org/apache/samza/util/Util.scala, line 336 > > <https://reviews.apache.org/r/53297/diff/1/?file=1549405#file1549405line336> > > > > This edit looks like a mistake. > > > > Did this file need to be modified at all?
Idea was showing error because of that tag. It's not required for this patch. Removed. > On Nov. 5, 2016, 6:47 p.m., Jake Maes wrote: > > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java, line > > 73 > > <https://reviews.apache.org/r/53297/diff/1/?file=1549407#file1549407line73> > > > > What's the purpose of this static factory? > > > > The typical reason is to construct the object differently (e.g. a > > different subclass) based on some parameter. > > > > I don't see any value of the approach here. > > > > If there is some value, then the constructor should be made private. Discussed offline. It was harder to test SamzaRestService with all new operator instantiations happening in the constructor. Hence, mocking out these dependencies were harder. Also with this approach, looking at the constructor we get to know the dependencies of SamzaRestService explicitly. However, since there are no use cases which would require this factory, removing this factory. Keeping the constructor as it is, moving the instantiation into the main method. > On Nov. 5, 2016, 6:47 p.m., Jake Maes wrote: > > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java, line > > 75 > > <https://reviews.apache.org/r/53297/diff/1/?file=1549407#file1549407line75> > > > > I don't think the MetricsConfig constructure takes a subset. > > > > I think it takes the root and expects to find the "metrics" prefix > > Jake Maes wrote: > s/constructure/constructor > > phonetic brain fail Yes, that's true. It expects the root and finds the metrics prefix. Hence, stripPrefix is passed on as false, so that prefix isn't removed. This just selects the config subset that starts with METRICS_PREFIX, without removing the prefix. The goal is to not pass on the entire config object and pass only metrics related config into MetricsConfig constructor. > On Nov. 5, 2016, 6:47 p.m., Jake Maes wrote: > > samza-core/src/main/scala/org/apache/samza/util/MetricsReporterLoader.scala, > > line 40 > > <https://reviews.apache.org/r/53297/diff/1/?file=1549404#file1549404line40> > > > > We're moving away from Scala. All new files should be Java. Done. Migrated from scala to java. > On Nov. 5, 2016, 6:47 p.m., Jake Maes wrote: > > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java, line > > 60 > > <https://reviews.apache.org/r/53297/diff/1/?file=1549407#file1549407line60> > > > > The Resources will eventually emit metrics too, so I think this value > > is too specific. Changed it to SamzaRest. Not sure of the implications of using a proper name here. For instance, in SamzaContainerMetrics source string is assigned to value "unknown". I'm most certain that this string is a placeholder to register MetricsRegistry instances with MetricsReporter and not used when reporting the actual metrics. - Shanthoosh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53297/#review155061 ----------------------------------------------------------- On Nov. 8, 2016, 11:13 p.m., Shanthoosh Venkataraman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53297/ > ----------------------------------------------------------- > > (Updated Nov. 8, 2016, 11:13 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 > >
