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 51703: Enable passing of Configs and MetricsRegistry into Monitor objects.
Date Wed, 14 Sep 2016 20:54:37 GMT


> On Sept. 13, 2016, 3:53 p.m., Jake Maes wrote:
> > samza-rest/src/main/java/org/apache/samza/monitor/config/PropertiesMonitorConfigFactory.java,
line 37
> > <https://reviews.apache.org/r/51703/diff/1/?file=1493642#file1493642line37>
> >
> >     I think there are a couple high-level issues with the MonitorConfigFactory as
implemented here. 
> >     
> >     1. It seems to assume that the monitor config could be a separate file from
the rest of the samza rest service. I don't think this is a requirement and would add complexity.

> >     2. Related to 1, if there's only 1 config file, then there's no need to parse
it twice. Why not take the full parsed Config object from the samza rest service and just
subset it? Then there's no need for another config factory.

Updated the code to build the monitor configurations from the single config file in samza
rest.


> On Sept. 13, 2016, 3:53 p.m., Jake Maes wrote:
> > samza-rest/src/main/java/org/apache/samza/monitor/DefaultMonitorFactory.java, line
41
> > <https://reviews.apache.org/r/51703/diff/1/?file=1493636#file1493636line41>
> >
> >     Why not have the user specify the factory class instead of the monitor class?
The factory is much more flexible. For example, a user-defined factory could instantiate a
monitor and then set its config values (i.e. doesn't require a constructor that takes Config
and MetricsRegistry). Not that I'm advocating that pattern of creating monitors; my point
is that a user-defined factory can make those kinds of tradeoffs.

Rational behind the decision was to group the implementation details and the configurations
related to a monitor at a single place. (monitor.class, monitor.scheduling.interval etc.)
However, this would impose a restriction of having one particular kind of MonitorFactory implementation.


This problem is removed by exposing a getSchedulingInterval method in Monitor interface. Now
each MonitorFactory implementation should be associated with a single Monitor implementation.
Now, there could be many different kind of MonitorFactory implementations.


- Shanthoosh


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


On Sept. 14, 2016, 8:37 p.m., Shanthoosh Venkataraman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51703/
> -----------------------------------------------------------
> 
> (Updated Sept. 14, 2016, 8:37 p.m.)
> 
> 
> Review request for samza, Boris Shkolnik, Jake Maes, Navina Ramesh, Jagadish Venkatraman,
Xinyu Liu, and Yi Pan (Data Infrastructure).
> 
> 
> Repository: samza
> 
> 
> Description
> -------
> 
> This patch aims at adding the following functionalities to Samza-Rest monitors.
> 
> * Schedule different monitors at different intervals of time.
> * Define custom monitor configurations and pass config along to the monitor 
> objects.
> 
> 
> Diffs
> -----
> 
>   docs/learn/documentation/versioned/rest/monitors.md 9833068d9f542b80bb438db156a10c85d4d53097

>   docs/learn/documentation/versioned/rest/overview.md 5b958accf4e1a3f05b949e9dc6e2e19a453523ca

>   samza-rest/src/main/java/org/apache/samza/monitor/AbstractMonitor.java PRE-CREATION

>   samza-rest/src/main/java/org/apache/samza/monitor/Monitor.java d69df5f73dbf2c494183f9dcc8061c20878742aa

>   samza-rest/src/main/java/org/apache/samza/monitor/MonitorLoader.java 502ecc49f32b4563e8ceb4ddfa6bc2542c60819e

>   samza-rest/src/main/java/org/apache/samza/monitor/SamzaMonitorService.java 2f4d9ddb76369c5e83d39152d492807dfb164981

>   samza-rest/src/main/java/org/apache/samza/monitor/SchedulingProvider.java aea1a9291e651660c798cabf59fcf0c0623bcbd0

>   samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java 6f5c10ac89523626c7f7e05558422daad2ccd4e8

>   samza-rest/src/test/java/org/apache/samza/monitor/TestMonitorService.java 1da343012b85f96f837e3cbf9a54ced3b29fede6

>   samza-rest/src/test/java/org/apache/samza/monitor/mock/DummyMonitor.java 8621db1b0e8ce3279cc8a5cb3a21bd137d442034

>   samza-rest/src/test/java/org/apache/samza/monitor/mock/ExceptionThrowingMonitor.java
c4f3f735f78d56f8bb3ef203a05e2bec92489767 
> 
> Diff: https://reviews.apache.org/r/51703/diff/
> 
> 
> Testing
> -------
> 
> Unit tests are used to verify the intended functionality.
> 
> 
> Thanks,
> 
> Shanthoosh Venkataraman
> 
>


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