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 Fri, 23 Sep 2016 17:06:18 GMT

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



Still reviewing. Here's some early feedback.


samza-rest/src/main/java/org/apache/samza/monitor/AbstractMonitor.java (lines 33 - 35)
<https://reviews.apache.org/r/51703/#comment218063>

    These properties should be exposed in a config class and accessed via that class. It's
harder to track them down if they're interspersed everywhere.
    
    In fact, looking at what it does, this is more of a MonitorConfig class than AbstractMonitor.



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

    Is this config parameter expected to be the entire config or a subset which is specific
to a monitor instance? If the latter, it should be made clear with some java doc.



samza-rest/src/main/java/org/apache/samza/monitor/MonitorLoader.java (line 31)
<https://reviews.apache.org/r/51703/#comment218088>

    Fill the return attribute on the javadoc, please. 
    
    You could move the text from line 30 to here.



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

    While I also prefer "create" the convention in Samza is for factories to expose a "get"
method.



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

    Metrics will probably be needed in Resources too. I think it will be better to receive
the metrics registry instance in the constructor.



samza-rest/src/main/java/org/apache/samza/monitor/SamzaMonitorService.java (lines 46 - 51)
<https://reviews.apache.org/r/51703/#comment218090>

    It's better to split any functionality pertaining to config parsing out into a separate
class.



samza-rest/src/test/java/org/apache/samza/monitor/TestMonitorService.java 
<https://reviews.apache.org/r/51703/#comment218093>

    Where's the corresponding test to verify that we instantiate monitors properly?



samza-rest/src/test/java/org/apache/samza/monitor/TestMonitorService.java (line 43)
<https://reviews.apache.org/r/51703/#comment218092>

    I don't think this test actually verifies anything anymore. It's supposed to prove that
an exception in a monitor instance will not cause the samza rest service to fail (via the
ExceptionThrowingMonitor)


- Jake Maes


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