samza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jake Maes <jacob.m...@gmail.com>
Subject Re: Review Request 51703: Enable passing of Configs and MetricsRegistry into Monitor objects.
Date Tue, 13 Sep 2016 15:53:32 GMT

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



Good patch. 

Doc updates are missing. 

Some initial feedback below.


samza-rest/src/main/java/org/apache/samza/monitor/DefaultMonitorFactory.java (line 41)
<https://reviews.apache.org/r/51703/#comment216233>

    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.



samza-rest/src/main/java/org/apache/samza/monitor/SamzaMonitorService.java (line 60)
<https://reviews.apache.org/r/51703/#comment216232>

    nit: We have plans, that's why we're adding this placeholder. We just aren't doing it
yet.



samza-rest/src/main/java/org/apache/samza/monitor/SamzaMonitorService.java (line 65)
<https://reviews.apache.org/r/51703/#comment216231>

    This can be info level since it will only be printed once for each monitor and could be
useful for users to confirm their monitor configs were interpreted properly.



samza-rest/src/main/java/org/apache/samza/monitor/SamzaMonitorService.java (line 105)
<https://reviews.apache.org/r/51703/#comment216235>

    You might need to rebase to pick up SAMZA-1005, which provides a standard way to instantiate
classes. https://reviews.apache.org/r/51726/



samza-rest/src/main/java/org/apache/samza/monitor/SamzaMonitorService.java (line 126)
<https://reviews.apache.org/r/51703/#comment216236>

    Same instantiation commment here. SAMZA-1005



samza-rest/src/main/java/org/apache/samza/monitor/config/MonitorConfig.java (line 33)
<https://reviews.apache.org/r/51703/#comment216237>

    I don't think we need this, if the user is providing a factory class name. The factory
should be user-defined code that just instantiate whatever monitors it wants.



samza-rest/src/main/java/org/apache/samza/monitor/config/PropertiesMonitorConfigFactory.java
(line 37)
<https://reviews.apache.org/r/51703/#comment216239>

    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.



samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java (line 53)
<https://reviews.apache.org/r/51703/#comment216238>

    monitor.factory.classes would be better. It makes it clear that one or more class names
are expected. 
    
    BTW, I think users should be able to provide more than one factory.



samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java (line 69)
<https://reviews.apache.org/r/51703/#comment216240>

    As mentioned above, I don't think this is necessary. It's simpler to have 1 config file
for the entire rest service, parse it once, and subset it for the various components.


- Jake Maes


On Sept. 7, 2016, 9:03 p.m., Shanthoosh Venkataraman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51703/
> -----------------------------------------------------------
> 
> (Updated Sept. 7, 2016, 9:03 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.
> * Default implementation of building monitor configuration from properties file.
> 
> 
> Diffs
> -----
> 
>   samza-rest/src/main/java/org/apache/samza/monitor/DefaultMonitorFactory.java PRE-CREATION

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

>   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/monitor/config/MonitorConfig.java PRE-CREATION

>   samza-rest/src/main/java/org/apache/samza/monitor/config/MonitorConfigFactory.java
PRE-CREATION 
>   samza-rest/src/main/java/org/apache/samza/monitor/config/PropertiesMonitorConfigFactory.java
PRE-CREATION 
>   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/config/TestPropertiesMonitorConfigFactory.java
PRE-CREATION 
>   samza-rest/src/test/java/org/apache/samza/monitor/mock/DummyMonitor.java 8621db1b0e8ce3279cc8a5cb3a21bd137d442034

>   samza-rest/src/test/java/org/apache/samza/monitor/mock/DummyMonitorConfigFactory.java
PRE-CREATION 
>   samza-rest/src/test/java/org/apache/samza/monitor/mock/ExceptionThrowingMonitor.java
c4f3f735f78d56f8bb3ef203a05e2bec92489767 
>   samza-rest/src/test/resources/monitorconfig.properties PRE-CREATION 
> 
> 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