samza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Shanthoosh Venkataraman <santhoshvenkat1...@gmail.com>
Subject Re: Review Request 53297: Initial version of adding metrics into samza rest.
Date Tue, 08 Nov 2016 23:13:18 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message