You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@samza.apache.org by Shanthoosh Venkataraman <sa...@gmail.com> on 2016/10/18 22:53:52 UTC
Review Request 53002: Do not load the monitor,
if the MonitorFactoryClass is not defined for the monitor in the config.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53002/
-----------------------------------------------------------
Review request for samza and Jake Maes.
Repository: samza
Description
-------
This patch aims to not load the monitors if the monitor factory class is not defined(set) for the monitor in the config. This will enable the users to turn on/off the monitors in samza-rest easily(just by setting the monitorFactoryClass config associated with monitor a to empty string.)
Diffs
-----
samza-rest/src/main/java/org/apache/samza/monitor/SamzaMonitorService.java ce947f7ae1175acc1ee9aa75991c726848072694
samza-rest/src/test/java/org/apache/samza/monitor/TestMonitorService.java 4618b54f5af861383df45bf7185622d36d17cd5e
samza-rest/src/test/java/org/apache/samza/monitor/mock/MockMonitorFactory.java PRE-CREATION
Diff: https://reviews.apache.org/r/53002/diff/
Testing
-------
Unit testing and manual testing are done to verify the functionality.
Thanks,
Shanthoosh Venkataraman
Re: Review Request 53002: Do not load the monitor,
if the MonitorFactoryClass is not defined for the monitor in the config.
Posted by Jake Maes <jm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53002/#review153224
-----------------------------------------------------------
Fix it, then Ship it!
One minor issue below.
Once that's fixed and the patch is attached to the JIRA, I'll commit it.
samza-rest/src/main/java/org/apache/samza/monitor/SamzaMonitorService.java (line 63)
<https://reviews.apache.org/r/53002/#comment222509>
Commons is not an explicit dependency of samza-rest in the build.gradle file, but Guava is. Can this be done with Strings?
https://google.github.io/guava/releases/19.0/api/docs/com/google/common/base/Strings.html
Commons is probably a transitive dependency. So while, we could get away with it now, it could break later.
- Jake Maes
On Oct. 18, 2016, 10:53 p.m., Shanthoosh Venkataraman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53002/
> -----------------------------------------------------------
>
> (Updated Oct. 18, 2016, 10:53 p.m.)
>
>
> Review request for samza and Jake Maes.
>
>
> Repository: samza
>
>
> Description
> -------
>
> This patch aims to not load the monitors if the monitor factory class is not defined(set) for the monitor in the config. This will enable the users to turn on/off the monitors in samza-rest easily(just by setting the monitorFactoryClass config associated with monitor a to empty string.)
>
>
> Diffs
> -----
>
> samza-rest/src/main/java/org/apache/samza/monitor/SamzaMonitorService.java ce947f7ae1175acc1ee9aa75991c726848072694
> samza-rest/src/test/java/org/apache/samza/monitor/TestMonitorService.java 4618b54f5af861383df45bf7185622d36d17cd5e
> samza-rest/src/test/java/org/apache/samza/monitor/mock/MockMonitorFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/53002/diff/
>
>
> Testing
> -------
>
> Unit testing and manual testing are done to verify the functionality.
>
>
> Thanks,
>
> Shanthoosh Venkataraman
>
>
Re: Review Request 53002: Do not load the monitor,
if the MonitorFactoryClass is not defined for the monitor in the config.
Posted by Shanthoosh Venkataraman <sa...@gmail.com>.
> On Oct. 18, 2016, 11:11 p.m., Fred Ji wrote:
> > samza-rest/src/main/java/org/apache/samza/monitor/SamzaMonitorService.java, line 71
> > <https://reviews.apache.org/r/53002/diff/1/?file=1540988#file1540988line71>
> >
> > nit: this line of comment is not needed since log warn is very clear.
Removed.
- Shanthoosh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53002/#review153208
-----------------------------------------------------------
On Oct. 19, 2016, 1:11 a.m., Shanthoosh Venkataraman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53002/
> -----------------------------------------------------------
>
> (Updated Oct. 19, 2016, 1:11 a.m.)
>
>
> Review request for samza and Jake Maes.
>
>
> Repository: samza
>
>
> Description
> -------
>
> This patch aims to not load the monitors if the monitor factory class is not defined(set) for the monitor in the config. This will enable the users to turn on/off the monitors in samza-rest easily(just by setting the monitorFactoryClass config associated with monitor a to empty string.)
>
>
> Diffs
> -----
>
> build.gradle 98839f2d44569cfe04f12207117668afd593b4df
> samza-rest/src/main/java/org/apache/samza/monitor/SamzaMonitorService.java ce947f7ae1175acc1ee9aa75991c726848072694
> samza-rest/src/test/java/org/apache/samza/monitor/TestMonitorService.java 4618b54f5af861383df45bf7185622d36d17cd5e
> samza-rest/src/test/java/org/apache/samza/monitor/mock/MockMonitorFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/53002/diff/
>
>
> Testing
> -------
>
> Unit testing and manual testing are done to verify the functionality.
>
>
> Thanks,
>
> Shanthoosh Venkataraman
>
>
Re: Review Request 53002: Do not load the monitor,
if the MonitorFactoryClass is not defined for the monitor in the config.
Posted by Fred Ji <fj...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53002/#review153208
-----------------------------------------------------------
Ship it!
samza-rest/src/main/java/org/apache/samza/monitor/SamzaMonitorService.java (line 71)
<https://reviews.apache.org/r/53002/#comment222498>
nit: this line of comment is not needed since log warn is very clear.
- Fred Ji
On Oct. 18, 2016, 10:53 p.m., Shanthoosh Venkataraman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53002/
> -----------------------------------------------------------
>
> (Updated Oct. 18, 2016, 10:53 p.m.)
>
>
> Review request for samza and Jake Maes.
>
>
> Repository: samza
>
>
> Description
> -------
>
> This patch aims to not load the monitors if the monitor factory class is not defined(set) for the monitor in the config. This will enable the users to turn on/off the monitors in samza-rest easily(just by setting the monitorFactoryClass config associated with monitor a to empty string.)
>
>
> Diffs
> -----
>
> samza-rest/src/main/java/org/apache/samza/monitor/SamzaMonitorService.java ce947f7ae1175acc1ee9aa75991c726848072694
> samza-rest/src/test/java/org/apache/samza/monitor/TestMonitorService.java 4618b54f5af861383df45bf7185622d36d17cd5e
> samza-rest/src/test/java/org/apache/samza/monitor/mock/MockMonitorFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/53002/diff/
>
>
> Testing
> -------
>
> Unit testing and manual testing are done to verify the functionality.
>
>
> Thanks,
>
> Shanthoosh Venkataraman
>
>
Re: Review Request 53002: Do not load the monitor,
if the MonitorFactoryClass is not defined for the monitor in the config.
Posted by Shanthoosh Venkataraman <sa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53002/
-----------------------------------------------------------
(Updated Oct. 19, 2016, 6 p.m.)
Review request for samza and Jake Maes.
Repository: samza
Description (updated)
-------
This patch aims to not load the monitors if the monitor factory class is not defined(set) for the monitor in the config. This will enable the users to turn on/off the monitors in samza-rest easily(just by setting the monitorFactoryClass config associated with monitor a to empty string.). This is associated with the JIRA ticket : https://issues.apache.org/jira/browse/SAMZA-1039
Diffs
-----
build.gradle 98839f2d44569cfe04f12207117668afd593b4df
samza-rest/src/main/java/org/apache/samza/monitor/SamzaMonitorService.java ce947f7ae1175acc1ee9aa75991c726848072694
samza-rest/src/test/java/org/apache/samza/monitor/TestMonitorService.java 4618b54f5af861383df45bf7185622d36d17cd5e
samza-rest/src/test/java/org/apache/samza/monitor/mock/MockMonitorFactory.java PRE-CREATION
Diff: https://reviews.apache.org/r/53002/diff/
Testing
-------
Unit testing and manual testing are done to verify the functionality.
Thanks,
Shanthoosh Venkataraman
Re: Review Request 53002: Do not load the monitor,
if the MonitorFactoryClass is not defined for the monitor in the config.
Posted by Shanthoosh Venkataraman <sa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53002/
-----------------------------------------------------------
(Updated Oct. 19, 2016, 1:11 a.m.)
Review request for samza and Jake Maes.
Repository: samza
Description
-------
This patch aims to not load the monitors if the monitor factory class is not defined(set) for the monitor in the config. This will enable the users to turn on/off the monitors in samza-rest easily(just by setting the monitorFactoryClass config associated with monitor a to empty string.)
Diffs (updated)
-----
build.gradle 98839f2d44569cfe04f12207117668afd593b4df
samza-rest/src/main/java/org/apache/samza/monitor/SamzaMonitorService.java ce947f7ae1175acc1ee9aa75991c726848072694
samza-rest/src/test/java/org/apache/samza/monitor/TestMonitorService.java 4618b54f5af861383df45bf7185622d36d17cd5e
samza-rest/src/test/java/org/apache/samza/monitor/mock/MockMonitorFactory.java PRE-CREATION
Diff: https://reviews.apache.org/r/53002/diff/
Testing
-------
Unit testing and manual testing are done to verify the functionality.
Thanks,
Shanthoosh Venkataraman