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 Fri, 11 Nov 2016 00:22:49 GMT


> On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java, line 27
> > <https://reviews.apache.org/r/53297/diff/5/?file=1560179#file1560179line27>
> >
> >     Unused import.

Removed.


> On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java, line 164
> > <https://reviews.apache.org/r/53297/diff/5/?file=1560180#file1560180line164>
> >
> >     debug?

I think this should be info. It will be useful to know the registered reporters while debugging.


> On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote:
> > samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java, line
39
> > <https://reviews.apache.org/r/53297/diff/5/?file=1560176#file1560176line39>
> >
> >     I meant remove the whole javadoc including @params. There's no useful additional
information here.

Removed.


> On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java, line 107
> > <https://reviews.apache.org/r/53297/diff/5/?file=1560180#file1560180line107>
> >
> >     Should this be called during start()? Same for stop().

To accomplish this, both(SamzaRestService,SamzaMonitorService) of them had to be coupled.
Logically they are seperate entities. One responsible for resources and other monitors. Currently
MonitorService & RestService are not coupled with each other. It would be useful keep
them seperate(In the future, we could have SamzaRestService and SamzaMonitorService deployed
seperately when we have too many(Monitors & Resources) in them). I would prefer not to
have this coupling.


> On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote:
> > samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala,
line 56
> > <https://reviews.apache.org/r/53297/diff/5/?file=1560182#file1560182line56>
> >
> >     Maybe use JavaCoverters._ and .asScala?

Done.


> On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote:
> > samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala,
line 46
> > <https://reviews.apache.org/r/53297/diff/5/?file=1560178#file1560178line46>
> >
> >     Maybe use JavaCoverters._ and .asScala?

Done.


> On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote:
> > samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java, line
30
> > <https://reviews.apache.org/r/53297/diff/5/?file=1560176#file1560176line30>
> >
> >     Remove "contains reusable methods which" and "based upon configuration". Don't
mind removing the whole comment either.

Removed.


> On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java, line 103
> > <https://reviews.apache.org/r/53297/diff/5/?file=1560180#file1560180line103>
> >
> >     Don't understand this interface. What's a SchedulingProvider, and how is it
different than a ScheduledExecutorService?
> >     
> >     schedulingProvider should be probably be stopped in stop() in this class and
not in SamzaMonitorService since this class owns it. Either that, or move instantiation to
SamzaMonitorService too.

SchedulingProvider interface looks similar to that of ExecutorService. Not sure of the value
add this interface provides. I guess, this is not required and we could just use ExeuctorService.
Added a jira for it, since it's not in the scope of this patch. We could re-visit it later.


> On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java, line 166
> > <https://reviews.apache.org/r/53297/diff/5/?file=1560180#file1560180line166>
> >
> >     debug?

I think this should be info. Will be useful to know this when debugging.


> On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote:
> > docs/learn/documentation/versioned/rest/monitors.md, line 98
> > <https://reviews.apache.org/r/53297/diff/5/?file=1560175#file1560175line98>
> >
> >     Maybe: "are the same as that of Samza jobs." and make "that of Samza jobs" the
configuration link and remove next line?

Done.


> On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java, line 91
> > <https://reviews.apache.org/r/53297/diff/5/?file=1560180#file1560180line91>
> >
> >     See previous comment about just passing the entire config. Does that not work?

Done.


- Shanthoosh


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


On Nov. 10, 2016, 1:04 a.m., Shanthoosh Venkataraman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53297/
> -----------------------------------------------------------
> 
> (Updated Nov. 10, 2016, 1:04 a.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