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