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