samza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Prateek Maheshwari <pmaheshw...@linkedin.com>
Subject Re: Review Request 53297: Initial version of adding metrics into samza rest.
Date Wed, 09 Nov 2016 00:31:38 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53297/#review155380
-----------------------------------------------------------




docs/learn/documentation/versioned/rest/monitors.md (line 100)
<https://reviews.apache.org/r/53297/#comment225263>

    This section could be more concise.



samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java (line 36)
<https://reviews.apache.org/r/53297/#comment225264>

    Minor: private constructors for helper classes are pretty universal, don't think they
need a comment.



samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java (line 40)
<https://reviews.apache.org/r/53297/#comment225265>

    This javadoc doesn't add any more information than the method signature already provides.
Prefer removing.



samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java (line 45)
<https://reviews.apache.org/r/53297/#comment225266>

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



samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java (line 55)
<https://reviews.apache.org/r/53297/#comment225267>

    What is this config, and what does container refer to here? The samza-rest service container?


- Prateek Maheshwari


On Nov. 8, 2016, 3: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, 3: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