samza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jake Maes <jma...@apache.org>
Subject Re: Review Request 51703: SAMZA-1024 : Enable passing of Configs and MetricsRegistry into Monitor objects.
Date Mon, 26 Sep 2016 19:37:42 GMT

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




docs/learn/documentation/versioned/rest/monitors.md (line 28)
<https://reviews.apache.org/r/51703/#comment218418>

    This paragraph begins the same way as the first one. It's a little repetitive. Can it
be reworded?
    
    "the creation of" is unnecessary.
    
    The word "individual" is unnecessary here.
    
    In general, this paragraph isn't very user-friendly. As a user, I just want it to tell
me what I can do and how to do it.



docs/learn/documentation/versioned/rest/monitors.md (line 30)
<https://reviews.apache.org/r/51703/#comment218417>

    Please follow the instructions under /docs/README.md to test the site. 
    
    In particular, this file doesn't build.
    
    ```
    docs[master] > bundle exec jekyll build
    Configuration file: /.../samza/samza/docs/_config.yml
                Source: /.../samza/samza/docs
           Destination: /.../samza/samza/docs/_site
          Generating... 
      Liquid Exception: Unknown tag 'monitorName' in learn/documentation/versioned/rest/monitors.md
    jekyll 2.0.3 | Error:  Unknown tag 'monitorName'
    ```



docs/learn/documentation/versioned/rest/monitors.md (line 33)
<https://reviews.apache.org/r/51703/#comment218421>

    "individual" is unnecessary here
    
    "should be defined" == "are required"



docs/learn/documentation/versioned/rest/monitors.md (lines 82 - 85)
<https://reviews.apache.org/r/51703/#comment218423>

    This section feels out of place and is perhaps unnecessary because we don't yet support
metrics.



docs/learn/documentation/versioned/rest/monitors.md (line 91)
<https://reviews.apache.org/r/51703/#comment218422>

    "will be used to instantiate your Monitor."



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

    Doesn't this method belong in MonitorConfig?
    It has more to do with config structure than loading.



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

    Unused.



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

    Unused.



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

    I think it's importand to have some kind of follow-up test after instantiation to make
sure the monitor was actually instantiated and returned, rather than null, for example. 
    
    Minimum requirements would be
    1. not null
    2. instance of Monitor



samza-rest/src/test/java/org/apache/samza/monitor/TestMonitorService.java (lines 73 - 76)
<https://reviews.apache.org/r/51703/#comment218432>

    If I'm reading this correctly, this test depends on MapConfig to retain order of the configs,
and that may not hold true.


- Jake Maes


On Sept. 24, 2016, 12:34 a.m., Shanthoosh Venkataraman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51703/
> -----------------------------------------------------------
> 
> (Updated Sept. 24, 2016, 12:34 a.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/Monitor.java d69df5f73dbf2c494183f9dcc8061c20878742aa

>   samza-rest/src/main/java/org/apache/samza/monitor/MonitorConfig.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/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/main/java/org/apache/samza/rest/SamzaRestService.java 5b34da814985fb09281f36c677a97372cacc7a75

>   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/DummyMonitorFactory.java PRE-CREATION

>   samza-rest/src/test/java/org/apache/samza/monitor/mock/ExceptionThrowingMonitor.java
c4f3f735f78d56f8bb3ef203a05e2bec92489767 
>   samza-rest/src/test/java/org/apache/samza/monitor/mock/ExceptionThrowingMonitorFactory.java
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