----------------------------------------------------------- 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 > >
