Review Request 51703: Enable passing of Configs and MetricsRegistry into Monitor objects.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51703/ --- Review request for samza. 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
Re: Review Request 51703: Enable passing of Configs and MetricsRegistry into Monitor objects.
--- 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 (updated) --- 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 (updated) - 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
Re: Review Request 51703: Enable passing of Configs and MetricsRegistry into Monitor objects.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51703/#review148764 --- samza-rest/src/main/java/org/apache/samza/monitor/DefaultMonitorFactory.java (line 41) <https://reviews.apache.org/r/51703/#comment216490> Rational behind the decision was to group the implementation detail and configurations related to a monitor at a single place. (monitor.class, monitor.scheduling.interval.ms etc.) However, this would impose a restriction of having one particular kind of MonitorFactory implementation. This problem is removed by exposing a getSchedulingInterval method in Monitor interface. Now each MonitorFactory implementation should be associated with a single Monitor implementation. There is more flexibility in allowing many custom MonitorFactory implementations. - Shanthoosh Venkataraman 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 > >
Re: Review Request 51703: Enable passing of Configs and MetricsRegistry into Monitor objects.
> On Sept. 13, 2016, 3:53 p.m., Jake Maes wrote: > > samza-rest/src/main/java/org/apache/samza/monitor/config/PropertiesMonitorConfigFactory.java, > > line 37 > > <https://reviews.apache.org/r/51703/diff/1/?file=1493642#file1493642line37> > > > > 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. Updated the code to build the monitor configurations from the single config file in samza rest. > On Sept. 13, 2016, 3:53 p.m., Jake Maes wrote: > > samza-rest/src/main/java/org/apache/samza/monitor/DefaultMonitorFactory.java, > > line 41 > > <https://reviews.apache.org/r/51703/diff/1/?file=1493636#file1493636line41> > > > > 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. Rational behind the decision was to group the implementation details and the configurations related to a monitor at a single place. (monitor.class, monitor.scheduling.interval etc.) However, this would impose a restriction of having one particular kind of MonitorFactory implementation. This problem is removed by exposing a getSchedulingInterval method in Monitor interface. Now each MonitorFactory implementation should be associated with a single Monitor implementation. Now, there could be many different kind of MonitorFactory implementations. - Shanthoosh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51703/#review148730 --- 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 > >
Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52168/ --- Review request for samza. Repository: samza Description --- This patch contains the following changes * Http get api to list the complete details of all the tasks that belongs to a job. * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream. Diffs - docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6 samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala ba38b5cfa4e61b5513ce38dd2be32438b62cd7ce samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31 samza-core/src/main/scala/org/apache/samza/util/Util.scala 95a5aa0a23db2a890c19166b6031b2a3b96689f2 samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545 samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4 samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382 samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089 samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797 samza-rest/src/main/java/org/apache/samza/rest/resources/ResourcesUtil.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockJobConfigFactory.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION Diff: https://reviews.apache.org/r/52168/diff/ Testing --- Manual and unit testing has been done to verify the apis. Thanks, Shanthoosh Venkataraman
Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52168/ --- (Updated Sept. 23, 2016, 5:57 p.m.) Review request for samza. Repository: samza Description --- This patch contains the following changes * Http get api to list the complete details of all the tasks that belongs to a job. * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream. Diffs (updated) - docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6 samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala ba38b5cfa4e61b5513ce38dd2be32438b62cd7ce samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31 samza-core/src/main/scala/org/apache/samza/util/Util.scala 95a5aa0a23db2a890c19166b6031b2a3b96689f2 samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545 samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4 samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382 samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089 samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797 samza-rest/src/main/java/org/apache/samza/rest/resources/ResourcesUtil.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION Diff: https://reviews.apache.org/r/52168/diff/ Testing --- Manual and unit testing has been done to verify the apis. Thanks, Shanthoosh Venkataraman
Re: Review Request 51703: Enable passing of Configs and MetricsRegistry into Monitor objects.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51703/#review150237 --- samza-rest/src/main/java/org/apache/samza/monitor/SamzaMonitorService.java (lines 74 - 75) <https://reviews.apache.org/r/51703/#comment218128> Instantiation of monitors through reflection using monitorFactoryClassName can possibly throw InstantiationException. This catch block handles that. - Shanthoosh Venkataraman 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 > >
Re: Review Request 51703: Enable passing of Configs and MetricsRegistry into Monitor objects.
> On Sept. 23, 2016, 8:33 p.m., Jake Maes wrote: > > samza-rest/src/main/java/org/apache/samza/monitor/MonitorLoader.java, line > > 34 > > <https://reviews.apache.org/r/51703/diff/2/?file=1498673#file1498673line34> > > > > Is it reasonable for this method to throw anything. e.g. > > InstantiationException? > > > > A unit test should show that if an exception occurs, the service > > handles it properly. > > > > Specifically the samza rest service should not fail because a new > > monitor is broken. It should definitely log it though. Added throws Exception to method signature. Failures that occurred in any one of the samza monitors instantiation would fail the samza service startup. > On Sept. 23, 2016, 8:33 p.m., Jake Maes wrote: > > samza-rest/src/main/java/org/apache/samza/monitor/SamzaMonitorService.java, > > lines 75-76 > > <https://reviews.apache.org/r/51703/diff/2/?file=1498674#file1498674line75> > > > > What purpose does this serve? Samza monitor instantiation that uses reflection to create the monitor instances from factory, can throw InstantiationException. This catch block handles that. - Shanthoosh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51703/#review150206 --- 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 > >
Re: Review Request 51703: Enable passing of Configs and MetricsRegistry into Monitor objects.
> On Sept. 23, 2016, 5:06 p.m., Jake Maes wrote: > > samza-rest/src/main/java/org/apache/samza/monitor/AbstractMonitor.java, > > lines 33-35 > > <https://reviews.apache.org/r/51703/diff/2/?file=1498671#file1498671line33> > > > > 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. Moved this configuration parsing into a seperate class MonitorConfig. > On Sept. 23, 2016, 5:06 p.m., Jake Maes wrote: > > samza-rest/src/test/java/org/apache/samza/monitor/TestMonitorService.java, > > line 40 > > <https://reviews.apache.org/r/51703/diff/2/?file=1498677#file1498677line40> > > > > Where's the corresponding test to verify that we instantiate monitors > > properly? Added a custom mock factory and a test case that tests instantiating monitors through it. > On Sept. 23, 2016, 5:06 p.m., Jake Maes wrote: > > samza-rest/src/main/java/org/apache/samza/monitor/SamzaMonitorService.java, > > lines 47-52 > > <https://reviews.apache.org/r/51703/diff/2/?file=1498674#file1498674line47> > > > > It's better to split any functionality pertaining to config parsing out > > into a separate class. Whole functionality of parsing the configs are migrated to MonitorLoader class. - Shanthoosh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51703/#review150196 --- 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 > >
Re: Review Request 51703: SAMZA-1024 : Enable passing of Configs and MetricsRegistry into Monitor objects.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51703/ --- (Updated Sept. 23, 2016, 11:19 p.m.) Review request for samza, Boris Shkolnik, Jake Maes, Navina Ramesh, Jagadish Venkatraman, Xinyu Liu, and Yi Pan (Data Infrastructure). Summary (updated) - SAMZA-1024 : Enable passing of Configs and MetricsRegistry into Monitor objects. 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 (updated) - 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/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
Re: Review Request 51703: SAMZA-1024 : Enable passing of Configs and MetricsRegistry into Monitor objects.
--- 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 (updated) - 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
Re: Review Request 51703: SAMZA-1024 : Enable passing of Configs and MetricsRegistry into Monitor objects.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51703/ --- (Updated Sept. 26, 2016, 9:13 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 (updated) - 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
Re: Review Request 51703: SAMZA-1024 : Enable passing of Configs and MetricsRegistry into Monitor objects.
> On Sept. 26, 2016, 7:37 p.m., Jake Maes wrote: > > samza-rest/src/test/java/org/apache/samza/monitor/TestMonitorService.java, > > lines 81-84 > > <https://reviews.apache.org/r/51703/diff/4/?file=1509611#file1509611line81> > > > > If I'm reading this correctly, this test depends on MapConfig to retain > > order of the configs, and that may not hold true. Order of the config does not matter here, since there are no common keys betwen two config maps. Combining two config maps into one config object, would not cause problem due to uniqueness of all the keys. > On Sept. 26, 2016, 7:37 p.m., Jake Maes wrote: > > samza-rest/src/test/java/org/apache/samza/monitor/TestMonitorService.java, > > lines 63-68 > > <https://reviews.apache.org/r/51703/diff/4/?file=1509611#file1509611line63> > > > > 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 Instance of Monitor is always true, since the factory returns Monitor object back. Added not null & monitor checks. - Shanthoosh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51703/#review150435 --- 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 > >
Re: Review Request 51703: SAMZA-1024 : Enable passing of Configs and MetricsRegistry into Monitor objects.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51703/ --- (Updated Sept. 26, 2016, 11:14 p.m.) Review request for samza, Boris Shkolnik, Jake Maes, Navina Ramesh, Jagadish Venkatraman, Xinyu Liu, and Yi Pan (Data Infrastructure). Repository: samza Description (updated) --- 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. JIRA ticket : https://issues.apache.org/jira/browse/SAMZA-1024 Diffs (updated) - 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
Re: Review Request 51703: SAMZA-1024 : Enable passing of Configs and MetricsRegistry into Monitor objects.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51703/ --- (Updated Sept. 27, 2016, 6:57 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. JIRA ticket : https://issues.apache.org/jira/browse/SAMZA-1024 Diffs (updated) - 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
Review Request 52476: Do not load task store which are older than delete tombstones.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52476/ --- Review request for samza. Repository: samza Description --- Every local task store is backed up by a kafka changelog topic. Due to log compaction, delete tombstones of the changelog topic have a ttl of delete.retention.ms. Replaying the events from the changelog that has missing delete tombstones, would result in creation of an inconsistent local store(due to the missing of some delete events). This patch deletes the local stores in which difference between current time and last modified time of the offset file is greater than delete.retention.ms during the container startup. Diffs - samza-core/src/main/java/org/apache/samza/storage/StorageRecovery.java 9329edf7d724f3a0d9235354bb77936f713e3b5f samza-core/src/main/scala/org/apache/samza/config/StorageConfig.scala be3f1068f7921c9c8f8697577efea6580672813e samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6 samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala 0b7bcdda1639eea8239a69c31bdf42558e9077d2 samza-core/src/test/scala/org/apache/samza/storage/TestTaskStorageManager.scala 4d40f520e54beb643acd8410c772b75e2f6a9162 Diff: https://reviews.apache.org/r/52476/diff/ Testing --- Unit testing and manual testing has been done to verify the functionality. Thanks, Shanthoosh Venkataraman
Review Request 52492: Adding monitor to clean up stale local stores of jobs/tasks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52492/ --- Review request for samza. Repository: samza Description --- This patch contains the samza-rest monitor that periodically cleans up the stale local stores of dead jobs/tasks. It performs the store deletion in two phases. Initially it deletes the offset file in the local task stores if the following condition is true. ((jobIsNotRunning || preferedHost != nmHost) && offsetFilelastModifiedTime is greater than deleteRetention). During the subsequent run, it deletes the local task stores if it does not contain offset file. Please refer to the design doc of SAMZA-656 (https://issues.apache.org/jira/secure/attachment/12828083/DESIGN-SAMZA-656.pdf) for more details. Diffs - build.gradle 2bea27b75288d3103178bc3762b9556f6e69cdd1 samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitorConfig.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitorFactory.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/monitor/TestLocalStoreMonitor.java PRE-CREATION Diff: https://reviews.apache.org/r/52492/diff/ Testing --- Unit testing and manual testing are done to verify the functionality. Thanks, Shanthoosh Venkataraman
Re: Review Request 52476: Do not load task store which are older than delete tombstones.
> On Oct. 4, 2016, 1:57 a.m., Jake Maes wrote: > > samza-core/src/test/scala/org/apache/samza/storage/TestTaskStorageManager.scala, > > line 293 > > <https://reviews.apache.org/r/52476/diff/1/?file=1518377#file1518377line293> > > > > 2 nits: > > 1. Can you swap this test with the next one in terms of position? The > > tests above and below this one are related, so this one breaks them up, > > which just adds cognitive load for the reader. > > 2. I'm all for descriptive names, but this is almost un-tweet-able. :-) > > Could it be shortened to: > > testStoreDeletedWhenOffsetFileOlderThanDeleteRetention() Fixed. - Shanthoosh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52476/#review151273 ------- On Oct. 3, 2016, 5 p.m., Shanthoosh Venkataraman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52476/ > --- > > (Updated Oct. 3, 2016, 5 p.m.) > > > Review request for samza. > > > Repository: samza > > > Description > --- > > Every local task store is backed up by a kafka changelog topic. Due to log > compaction, delete tombstones of the changelog topic have a ttl of > delete.retention.ms. Replaying the events from the changelog that has missing > delete tombstones, would result in creation of an inconsistent local > store(due to the missing of some delete events). This patch deletes the local > stores in which difference between current time and last modified time of the > offset file is greater than delete.retention.ms during the container startup. > > > Diffs > - > > samza-core/src/main/java/org/apache/samza/storage/StorageRecovery.java > 9329edf7d724f3a0d9235354bb77936f713e3b5f > samza-core/src/main/scala/org/apache/samza/config/StorageConfig.scala > be3f1068f7921c9c8f8697577efea6580672813e > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > 05a996c98075ea8ed3767af666b9beeb1933f2a6 > samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala > 0b7bcdda1639eea8239a69c31bdf42558e9077d2 > > samza-core/src/test/scala/org/apache/samza/storage/TestTaskStorageManager.scala > 4d40f520e54beb643acd8410c772b75e2f6a9162 > > Diff: https://reviews.apache.org/r/52476/diff/ > > > Testing > --- > > Unit testing and manual testing has been done to verify the functionality. > > > Thanks, > > Shanthoosh Venkataraman > >
Re: Review Request 52476: Do not load task store which are older than delete tombstones.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52476/ --- (Updated Oct. 4, 2016, 11:33 p.m.) Review request for samza. Repository: samza Description --- Every local task store is backed up by a kafka changelog topic. Due to log compaction, delete tombstones of the changelog topic have a ttl of delete.retention.ms. Replaying the events from the changelog that has missing delete tombstones, would result in creation of an inconsistent local store(due to the missing of some delete events). This patch deletes the local stores in which difference between current time and last modified time of the offset file is greater than delete.retention.ms during the container startup. Diffs (updated) - samza-core/src/main/java/org/apache/samza/storage/StorageRecovery.java 9329edf7d724f3a0d9235354bb77936f713e3b5f samza-core/src/main/scala/org/apache/samza/config/StorageConfig.scala be3f1068f7921c9c8f8697577efea6580672813e samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6 samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala 0b7bcdda1639eea8239a69c31bdf42558e9077d2 samza-core/src/test/scala/org/apache/samza/storage/TestTaskStorageManager.scala 4d40f520e54beb643acd8410c772b75e2f6a9162 Diff: https://reviews.apache.org/r/52476/diff/ Testing --- Unit testing and manual testing has been done to verify the functionality. Thanks, Shanthoosh Venkataraman
Re: Review Request 52492: Adding monitor to clean up stale local stores of jobs/tasks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52492/ --- (Updated Oct. 8, 2016, 12:07 a.m.) Review request for samza. Repository: samza Description --- This patch contains the samza-rest monitor that periodically cleans up the stale local stores of dead jobs/tasks. It performs the store deletion in two phases. Initially it deletes the offset file in the local task stores if the following condition is true. ((jobIsNotRunning || preferedHost != nmHost) && offsetFilelastModifiedTime is greater than deleteRetention). During the subsequent run, it deletes the local task stores if it does not contain offset file. Please refer to the design doc of SAMZA-656 (https://issues.apache.org/jira/secure/attachment/12828083/DESIGN-SAMZA-656.pdf) for more details. Diffs (updated) - build.gradle 2bea27b75288d3103178bc3762b9556f6e69cdd1 samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitorConfig.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitorFactory.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/monitor/TestLocalStoreMonitor.java PRE-CREATION Diff: https://reviews.apache.org/r/52492/diff/ Testing --- Unit testing and manual testing are done to verify the functionality. Thanks, Shanthoosh Venkataraman
Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52168/ --- (Updated Oct. 8, 2016, 12:12 a.m.) Review request for samza. Repository: samza Description --- This patch contains the following changes * Http get api to list the complete details of all the tasks that belongs to a job. * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream. Diffs (updated) - docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6 samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala ba38b5cfa4e61b5513ce38dd2be32438b62cd7ce samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31 samza-core/src/main/scala/org/apache/samza/util/Util.scala 95a5aa0a23db2a890c19166b6031b2a3b96689f2 samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545 samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4 samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382 samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089 samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797 samza-rest/src/main/java/org/apache/samza/rest/resources/ResourcesUtil.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION Diff: https://reviews.apache.org/r/52168/diff/ Testing --- Manual and unit testing has been done to verify the apis. Thanks, Shanthoosh Venkataraman
Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52168/ --- (Updated Oct. 10, 2016, 11:10 p.m.) Review request for samza. Repository: samza Description --- This patch contains the following changes * Http get api to list the complete details of all the tasks that belongs to a job. * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream. Diffs (updated) - docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6 samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala ba38b5cfa4e61b5513ce38dd2be32438b62cd7ce samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31 samza-core/src/main/scala/org/apache/samza/util/Util.scala 95a5aa0a23db2a890c19166b6031b2a3b96689f2 samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545 samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4 samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382 samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089 samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797 samza-rest/src/main/java/org/apache/samza/rest/resources/ResourcesUtil.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION Diff: https://reviews.apache.org/r/52168/diff/ Testing --- Manual and unit testing has been done to verify the apis. Thanks, Shanthoosh Venkataraman
Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job
> On Oct. 10, 2016, 6:55 p.m., Jake Maes wrote: > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java, > > lines 48-51 > > <https://reviews.apache.org/r/52168/diff/3/?file=1527780#file1527780line48> > > > > What is the point of this refactor? > > > > It seems to complicate the constructor signature with an unnecessary > > parameter (because the installationFinder can be retrieved from the config, > > so why pass both?) and it doesn't seem to enable any new code path. Reason was InstallationFinder is a pluggable interface, creating the simple yarn proxy based upon config & SimpleInstallationFinder is not an extensible idea. If a user has his own implementation of InstallationFinder, it is harder to reuse SimpleYarnJobProxy with that implementation(users are required to use SimpleInstallationfinder). If supporting this kind of extensibility is not a requirement, I will revert back this refactoring. Please let me know your thoughts. > On Oct. 10, 2016, 6:55 p.m., Jake Maes wrote: > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java, > > lines 128-130 > > <https://reviews.apache.org/r/52168/diff/3/?file=1527782#file1527782line128> > > > > This doesn't look right. > > > > It doesn't seem like we should generally need to overwrite the job name > > and id. > > > > Why shouldn't this be only applying the standard rewriters as in > > JobRunner.rewriteConfig()? > > > > Is this compensating for something unique to your environment? If so, > > it should be pushed down to some part of the pluggable API. Since job name & job id is required to retrieve job config from coordinator stream, it's unavailable in the job coordinator response. Even in samza-container startup, deployments provide necessary jobId, name configuration. Here we have jobId, jobName passed on as the api params, we could use it to populate the config. By pluggablity you mean plugging in (jobId, jobName) or the rewriters. - Shanthoosh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52168/#review152002 --- On Oct. 10, 2016, 11:10 p.m., Shanthoosh Venkataraman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52168/ > --- > > (Updated Oct. 10, 2016, 11:10 p.m.) > > > Review request for samza. > > > Repository: samza > > > Description > --- > > This patch contains the following changes > * Http get api to list the complete details of all the tasks that belongs to > a job. > * Refactored some methods in coordinator stream, to reuse the existing > functionality of getting jobConfig from the coordinator stream. > > > Diffs > - > > docs/learn/documentation/versioned/rest/resource-directory.md > 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef > docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > 05a996c98075ea8ed3767af666b9beeb1933f2a6 > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala > ba38b5cfa4e61b5513ce38dd2be32438b62cd7ce > samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala > 022b480856483059cb9f837a08f97a718bc04c31 > samza-core/src/main/scala/org/apache/samza/util/Util.scala > 95a5aa0a23db2a890c19166b6031b2a3b96689f2 > samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java > PRE-CREATION > samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java > 4d8647f3e1e650632e38b47ba5a8a2dac004f545 > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java > 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java > a935c98730f85f448c688a6baf2e8ddffdbb2cb4 > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java > 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java > PRE-CREATION > samza-rest/src/main/java/org/ap
Re: Review Request 52492: Adding monitor to clean up stale local stores of jobs/tasks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52492/ --- (Updated Oct. 11, 2016, 12:53 a.m.) Review request for samza. Repository: samza Description --- This patch contains the samza-rest monitor that periodically cleans up the stale local stores of dead jobs/tasks. It performs the store deletion in two phases. Initially it deletes the offset file in the local task stores if the following condition is true. ((jobIsNotRunning || preferedHost != nmHost) && offsetFilelastModifiedTime is greater than deleteRetention). During the subsequent run, it deletes the local task stores if it does not contain offset file. Please refer to the design doc of SAMZA-656 (https://issues.apache.org/jira/secure/attachment/12828083/DESIGN-SAMZA-656.pdf) for more details. Diffs (updated) - build.gradle 2bea27b75288d3103178bc3762b9556f6e69cdd1 samza-rest/src/main/java/org/apache/samza/monitor/YarnLocalStoreMonitor.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/monitor/YarnLocalStoreMonitorConfig.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/monitor/YarnLocalStoreMonitorFactory.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089 samza-rest/src/main/java/org/apache/samza/rest/resources/ResourceConstants.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/monitor/TestYarnLocalStoreMonitor.java PRE-CREATION Diff: https://reviews.apache.org/r/52492/diff/ Testing --- Unit testing and manual testing are done to verify the functionality. Thanks, Shanthoosh Venkataraman
Re: Review Request 52492: Adding monitor to clean up stale local stores of jobs/tasks.
> On Oct. 10, 2016, 7:27 p.m., Jake Maes wrote: > > samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java, > > line 84 > > <https://reviews.apache.org/r/52492/diff/2/?file=1527756#file1527756line84> > > > > Shouldn't this be checked only once in the constructor? Current monitor loading implementation doesn't load monitors that throws exceptions during instantiation. If we throw up in the constructor, the monitor won't even load and user may not be aware of the problem. If we don't validate in constructor, there's a good chance that the user might notice the problem seeing lot of exceptions getting thrown from monitor method. > On Oct. 10, 2016, 7:27 p.m., Jake Maes wrote: > > samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java, > > line 164 > > <https://reviews.apache.org/r/52492/diff/2/?file=1527756#file1527756line164> > > > > I think this class is mixing 2 concerns and it would be better to > > separate them. > > 1. How to determine the urls to query. > > 2. Expose a simple client to fetch info from the samza rest API at > > those urls Moved all the url constants to query into ResourceConstants class. - Shanthoosh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52492/#review152039 --- On Oct. 8, 2016, 12:07 a.m., Shanthoosh Venkataraman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52492/ > --- > > (Updated Oct. 8, 2016, 12:07 a.m.) > > > Review request for samza. > > > Repository: samza > > > Description > --- > > This patch contains the samza-rest monitor that periodically cleans up the > stale local stores of dead jobs/tasks. It performs the store deletion in two > phases. Initially it deletes the offset file in the local task stores if the > following condition is true. ((jobIsNotRunning || preferedHost != nmHost) && > offsetFilelastModifiedTime is greater than deleteRetention). During the > subsequent run, it deletes the local task stores if it does not contain > offset file. Please refer to the design doc of SAMZA-656 > (https://issues.apache.org/jira/secure/attachment/12828083/DESIGN-SAMZA-656.pdf) > for more details. > > > Diffs > - > > build.gradle 2bea27b75288d3103178bc3762b9556f6e69cdd1 > samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitorConfig.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitorFactory.java > PRE-CREATION > samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java > PRE-CREATION > samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION > > samza-rest/src/test/java/org/apache/samza/monitor/TestLocalStoreMonitor.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/52492/diff/ > > > Testing > --- > > Unit testing and manual testing are done to verify the functionality. > > > Thanks, > > Shanthoosh Venkataraman > >
Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52168/#review152412 --- samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (lines 128 - 130) <https://reviews.apache.org/r/52168/#comment221414> As we'd discussed, I've removed setting JobId, JobName which is linkedin specific in this patch. Any deployment environment can have it's own ConfigFactory implementation where it rewrites configuration specific to it. - Shanthoosh Venkataraman On Oct. 10, 2016, 11:10 p.m., Shanthoosh Venkataraman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52168/ > --- > > (Updated Oct. 10, 2016, 11:10 p.m.) > > > Review request for samza. > > > Repository: samza > > > Description > --- > > This patch contains the following changes > * Http get api to list the complete details of all the tasks that belongs to > a job. > * Refactored some methods in coordinator stream, to reuse the existing > functionality of getting jobConfig from the coordinator stream. > > > Diffs > - > > docs/learn/documentation/versioned/rest/resource-directory.md > 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef > docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > 05a996c98075ea8ed3767af666b9beeb1933f2a6 > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala > df63b97e9d598ecd1840111ba490a723e410d089 > samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala > 022b480856483059cb9f837a08f97a718bc04c31 > samza-core/src/main/scala/org/apache/samza/util/Util.scala > c4836f202f7eda1d4e71eac94fd48e46207b0316 > samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java > PRE-CREATION > samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java > 4d8647f3e1e650632e38b47ba5a8a2dac004f545 > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java > 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java > a935c98730f85f448c688a6baf2e8ddffdbb2cb4 > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java > 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java > PRE-CREATION > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java > e0224c6bcf4aeaa336e5786ac472482507fcd382 > samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java > a566db598c284d69ea61af88fdc0851483d5a089 > > samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java > 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797 > samza-rest/src/main/java/org/apache/samza/rest/resources/ResourcesUtil.java > PRE-CREATION > samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java > PRE-CREATION > > samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java > 7db437b348ecd286185898b8f8ab0220d59da71a > > samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java > PRE-CREATION > > samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java > PRE-CREATION > > samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java > PRE-CREATION > > samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/52168/diff/ > > > Testing > --- > > Manual and unit testing has been done to verify the apis. > > > Thanks, > > Shanthoosh Venkataraman > >
Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52168/ --- (Updated Oct. 12, 2016, 10:54 p.m.) Review request for samza. Repository: samza Description --- This patch contains the following changes * Http get api to list the complete details of all the tasks that belongs to a job. * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream. Diffs (updated) - docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6 samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089 samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31 samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316 samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545 samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4 samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382 samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089 samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797 samza-rest/src/main/java/org/apache/samza/rest/resources/ResourcesUtil.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION Diff: https://reviews.apache.org/r/52168/diff/ Testing --- Manual and unit testing has been done to verify the apis. Thanks, Shanthoosh Venkataraman
Re: Review Request 52492: Adding monitor to clean up stale local stores of jobs/tasks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52492/ --- (Updated Oct. 13, 2016, 9:01 p.m.) Review request for samza. Repository: samza Description --- This patch contains the samza-rest monitor that periodically cleans up the stale local stores of dead jobs/tasks. It performs the store deletion in two phases. Initially it deletes the offset file in the local task stores if the following condition is true. ((jobIsNotRunning || preferedHost != nmHost) && offsetFilelastModifiedTime is greater than deleteRetention). During the subsequent run, it deletes the local task stores if it does not contain offset file. Please refer to the design doc of SAMZA-656 (https://issues.apache.org/jira/secure/attachment/12828083/DESIGN-SAMZA-656.pdf) for more details. Diffs (updated) - build.gradle 2bea27b75288d3103178bc3762b9556f6e69cdd1 samza-rest/src/main/java/org/apache/samza/monitor/YarnLocalStoreMonitor.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/monitor/YarnLocalStoreMonitorConfig.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/monitor/YarnLocalStoreMonitorFactory.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/ResourceConstants.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/monitor/TestYarnLocalStoreMonitor.java PRE-CREATION Diff: https://reviews.apache.org/r/52492/diff/ Testing --- Unit testing and manual testing are done to verify the functionality. Thanks, Shanthoosh Venkataraman
Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52168/ --- (Updated Oct. 13, 2016, 11:57 p.m.) Review request for samza. Repository: samza Description --- This patch contains the following changes * Http get api to list the complete details of all the tasks that belongs to a job. * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream. Diffs (updated) - docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala 13b72fae7815ddaea7ae03a24f1a426ca51613cc samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6 samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089 samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31 samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316 samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545 samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4 samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382 samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089 samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797 samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION Diff: https://reviews.apache.org/r/52168/diff/ Testing --- Manual and unit testing has been done to verify the apis. Thanks, Shanthoosh Venkataraman
Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job
> On Oct. 13, 2016, 12:55 a.m., Jake Maes wrote: > > docs/learn/documentation/versioned/rest/resources/tasks.md, line 55 > > <https://reviews.apache.org/r/52168/diff/5/?file=1533539#file1533539line55> > > > > What is the value of the container name? name of the samza container in which the task is running. With respect to current implementation, container name is unique within a job. - Shanthoosh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52168/#review152418 --- On Oct. 13, 2016, 11:57 p.m., Shanthoosh Venkataraman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52168/ > --- > > (Updated Oct. 13, 2016, 11:57 p.m.) > > > Review request for samza. > > > Repository: samza > > > Description > --- > > This patch contains the following changes > * Http get api to list the complete details of all the tasks that belongs to > a job. > * Refactored some methods in coordinator stream, to reuse the existing > functionality of getting jobConfig from the coordinator stream. > > > Diffs > - > > docs/learn/documentation/versioned/rest/resource-directory.md > 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef > docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION > samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala > 13b72fae7815ddaea7ae03a24f1a426ca51613cc > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > 05a996c98075ea8ed3767af666b9beeb1933f2a6 > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala > df63b97e9d598ecd1840111ba490a723e410d089 > samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala > 022b480856483059cb9f837a08f97a718bc04c31 > samza-core/src/main/scala/org/apache/samza/util/Util.scala > c4836f202f7eda1d4e71eac94fd48e46207b0316 > samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java > PRE-CREATION > samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java > 4d8647f3e1e650632e38b47ba5a8a2dac004f545 > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java > 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java > a935c98730f85f448c688a6baf2e8ddffdbb2cb4 > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java > 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java > PRE-CREATION > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java > e0224c6bcf4aeaa336e5786ac472482507fcd382 > samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java > a566db598c284d69ea61af88fdc0851483d5a089 > > samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java > 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797 > samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java > PRE-CREATION > samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java > PRE-CREATION > > samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java > 7db437b348ecd286185898b8f8ab0220d59da71a > > samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java > PRE-CREATION > > samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java > PRE-CREATION > > samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java > PRE-CREATION > > samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java > PRE-CREATION > > samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/52168/diff/ > > > Testing > --- > > Manual and unit testing has been done to verify the apis. > > > Thanks, > > Shanthoosh Venkataraman > >
Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job
- Shanthoosh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52168/#review152418 --- On Oct. 13, 2016, 11:57 p.m., Shanthoosh Venkataraman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52168/ > --- > > (Updated Oct. 13, 2016, 11:57 p.m.) > > > Review request for samza. > > > Repository: samza > > > Description > --- > > This patch contains the following changes > * Http get api to list the complete details of all the tasks that belongs to > a job. > * Refactored some methods in coordinator stream, to reuse the existing > functionality of getting jobConfig from the coordinator stream. > > > Diffs > - > > docs/learn/documentation/versioned/rest/resource-directory.md > 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef > docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION > samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala > 13b72fae7815ddaea7ae03a24f1a426ca51613cc > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > 05a996c98075ea8ed3767af666b9beeb1933f2a6 > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala > df63b97e9d598ecd1840111ba490a723e410d089 > samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala > 022b480856483059cb9f837a08f97a718bc04c31 > samza-core/src/main/scala/org/apache/samza/util/Util.scala > c4836f202f7eda1d4e71eac94fd48e46207b0316 > samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java > PRE-CREATION > samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java > 4d8647f3e1e650632e38b47ba5a8a2dac004f545 > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java > 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java > a935c98730f85f448c688a6baf2e8ddffdbb2cb4 > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java > 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java > PRE-CREATION > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java > e0224c6bcf4aeaa336e5786ac472482507fcd382 > samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java > a566db598c284d69ea61af88fdc0851483d5a089 > > samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java > 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797 > samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java > PRE-CREATION > samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java > PRE-CREATION > > samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java > 7db437b348ecd286185898b8f8ab0220d59da71a > > samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java > PRE-CREATION > > samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java > PRE-CREATION > > samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java > PRE-CREATION > > samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java > PRE-CREATION > > samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/52168/diff/ > > > Testing > --- > > Manual and unit testing has been done to verify the apis. > > > Thanks, > > Shanthoosh Venkataraman > >
Re: Review Request 52476: Do not load task store which are older than delete tombstones.
> On Oct. 5, 2016, 2:08 a.m., Jake Maes wrote: > > Looks better, but I think there's still one major part missing. > > > > In order to have agreement between a kafka changelog and the task storage, > > the changelog should be created with the same delete.retention.ms property. > > > > There are 2 ways to do this: > > 1. (preferred) update the kafka system admin to read the samza changelog > > property that you've defined (which also needs to be added to the config > > table, btw) and create the topic with that value for delete.retention.ms > > 2. Rename the property so it's one of the "topic-level-property" so it gets > > automatically passed to kafka. This is convenient but wouldn't apply to > > other systems, which could be useful if those other systems have a delete > > retention policy. I think 1) is the only plausible way to accomplish this through job config. delete.retention.ms configuration is associated only with stores changelog, not applicable to topics in general, so making it topic level property might notbe a good idea. Enforcing the delete.retention.ms property is harder to accomplish through config, since kafka is a external system. Ideally, if there's a way to fetch kafka metadata/config(delete.retention.ms) about a topic, during container startups we could fetch that value, rather than expecting the users to specify it. - Shanthoosh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52476/#review151439 --- On Oct. 4, 2016, 11:33 p.m., Shanthoosh Venkataraman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52476/ > --- > > (Updated Oct. 4, 2016, 11:33 p.m.) > > > Review request for samza. > > > Repository: samza > > > Description > --- > > Every local task store is backed up by a kafka changelog topic. Due to log > compaction, delete tombstones of the changelog topic have a ttl of > delete.retention.ms. Replaying the events from the changelog that has missing > delete tombstones, would result in creation of an inconsistent local > store(due to the missing of some delete events). This patch deletes the local > stores in which difference between current time and last modified time of the > offset file is greater than delete.retention.ms during the container startup. > > > Diffs > - > > samza-core/src/main/java/org/apache/samza/storage/StorageRecovery.java > 9329edf7d724f3a0d9235354bb77936f713e3b5f > samza-core/src/main/scala/org/apache/samza/config/StorageConfig.scala > be3f1068f7921c9c8f8697577efea6580672813e > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > 05a996c98075ea8ed3767af666b9beeb1933f2a6 > samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala > 0b7bcdda1639eea8239a69c31bdf42558e9077d2 > > samza-core/src/test/scala/org/apache/samza/storage/TestTaskStorageManager.scala > 4d40f520e54beb643acd8410c772b75e2f6a9162 > > Diff: https://reviews.apache.org/r/52476/diff/ > > > Testing > --- > > Unit testing and manual testing has been done to verify the functionality. > > > Thanks, > > Shanthoosh Venkataraman > >
Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job
> On Oct. 13, 2016, 12:55 a.m., Jake Maes wrote:
> > docs/learn/documentation/versioned/rest/resources/tasks.md, line 55
> > <https://reviews.apache.org/r/52168/diff/5/?file=1533539#file1533539line55>
> >
> > What is the value of the container name?
>
> Shanthoosh Venkataraman wrote:
> name of the samza container in which the task is running. With respect to
> current implementation, container name is unique within a job.
>
> Jake Maes wrote:
> Sorry, the question was poorly worded. How is container name useful? Do
> we need it? If so, couldn't we derive it from the container id?
It will be used for debugging purposes in the monitor/client. It logically
belongs to the task hierarchy. To answer questions like finding list of
containers running on a particular host. These questions could be answered from
filter on the container name & preferred host. Adding this also in a way
completes the entire task model.
Currently container name is of the form samza-container-{containerId}. Hence,
we could derive it. But the logic used to generate container id is specific to
samza job model generator and is bound to change. Hence deriving it might not
be a good idea.
- Shanthoosh
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/#review152418
-------
On Oct. 13, 2016, 11:57 p.m., Shanthoosh Venkataraman wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52168/
> ---
>
> (Updated Oct. 13, 2016, 11:57 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> ---
>
> This patch contains the following changes
> * Http get api to list the complete details of all the tasks that belongs to
> a job.
> * Refactored some methods in coordinator stream, to reuse the existing
> functionality of getting jobConfig from the coordinator stream.
>
>
> Diffs
> -
>
> docs/learn/documentation/versioned/rest/resource-directory.md
> 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
> docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala
> 13b72fae7815ddaea7ae03a24f1a426ca51613cc
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala
> 05a996c98075ea8ed3767af666b9beeb1933f2a6
> samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala
> df63b97e9d598ecd1840111ba490a723e410d089
> samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala
> 022b480856483059cb9f837a08f97a718bc04c31
> samza-core/src/main/scala/org/apache/samza/util/Util.scala
> c4836f202f7eda1d4e71eac94fd48e46207b0316
> samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java
> PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
>
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java
> 4d8647f3e1e650632e38b47ba5a8a2dac004f545
>
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java
> 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
>
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java
> a935c98730f85f448c688a6baf2e8ddffdbb2cb4
>
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java
> 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
>
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java
> PRE-CREATION
>
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java
> PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java
> PRE-CREATION
>
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java
> PRE-CREATION
>
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java
> PRE-CREATION
>
> samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java
> PRE-CREATION
>
> samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java
> e0224c6bcf4aeaa336e5786ac472482507fcd382
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java
> a566db598c284d69ea61af88fdc0851483d5a089
>
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java
> 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
> samza-rest/src/main/ja
Re: Review Request 52476: Do not load task store which are older than delete tombstones.
> On Oct. 5, 2016, 2:08 a.m., Jake Maes wrote: > > Looks better, but I think there's still one major part missing. > > > > In order to have agreement between a kafka changelog and the task storage, > > the changelog should be created with the same delete.retention.ms property. > > > > There are 2 ways to do this: > > 1. (preferred) update the kafka system admin to read the samza changelog > > property that you've defined (which also needs to be added to the config > > table, btw) and create the topic with that value for delete.retention.ms > > 2. Rename the property so it's one of the "topic-level-property" so it gets > > automatically passed to kafka. This is convenient but wouldn't apply to > > other systems, which could be useful if those other systems have a delete > > retention policy. > > Shanthoosh Venkataraman wrote: > I think 1) is the only plausible way to accomplish this through job > config. delete.retention.ms configuration is associated only with stores > changelog, not applicable to topics in general, so making it topic level > property might notbe a good idea. Enforcing the delete.retention.ms property > is harder to accomplish through config, since kafka is a external system. > Ideally, if there's a way to fetch kafka metadata/config(delete.retention.ms) > about a topic, during container startups we could fetch that value, rather > than expecting the users to specify it. > > Jake Maes wrote: > Please take a look at > org.apache.samza.config.KafkaConfig#getChangelogKafkaProperties Done. - Shanthoosh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52476/#review151439 --- On Oct. 17, 2016, 10:40 p.m., Shanthoosh Venkataraman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52476/ > --- > > (Updated Oct. 17, 2016, 10:40 p.m.) > > > Review request for samza. > > > Repository: samza > > > Description > --- > > Every local task store is backed up by a kafka changelog topic. Due to log > compaction, delete tombstones of the changelog topic have a ttl of > delete.retention.ms. Replaying the events from the changelog that has missing > delete tombstones, would result in creation of an inconsistent local > store(due to the missing of some delete events). This patch deletes the local > stores in which difference between current time and last modified time of the > offset file is greater than delete.retention.ms during the container startup. > > > Diffs > - > > samza-core/src/main/java/org/apache/samza/storage/StorageRecovery.java > 9329edf7d724f3a0d9235354bb77936f713e3b5f > samza-core/src/main/scala/org/apache/samza/config/StorageConfig.scala > be3f1068f7921c9c8f8697577efea6580672813e > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > 05a996c98075ea8ed3767af666b9beeb1933f2a6 > samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala > 0b7bcdda1639eea8239a69c31bdf42558e9077d2 > > samza-core/src/test/scala/org/apache/samza/storage/TestTaskStorageManager.scala > 4d40f520e54beb643acd8410c772b75e2f6a9162 > samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala > 973ab8cfb3d248bec7efe5e338f5e667f097556d > > Diff: https://reviews.apache.org/r/52476/diff/ > > > Testing > --- > > Unit testing and manual testing has been done to verify the functionality. > > > Thanks, > > Shanthoosh Venkataraman > >
Re: Review Request 52476: Do not load task store which are older than delete tombstones.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52476/ --- (Updated Oct. 17, 2016, 10:40 p.m.) Review request for samza. Repository: samza Description --- Every local task store is backed up by a kafka changelog topic. Due to log compaction, delete tombstones of the changelog topic have a ttl of delete.retention.ms. Replaying the events from the changelog that has missing delete tombstones, would result in creation of an inconsistent local store(due to the missing of some delete events). This patch deletes the local stores in which difference between current time and last modified time of the offset file is greater than delete.retention.ms during the container startup. Diffs (updated) - samza-core/src/main/java/org/apache/samza/storage/StorageRecovery.java 9329edf7d724f3a0d9235354bb77936f713e3b5f samza-core/src/main/scala/org/apache/samza/config/StorageConfig.scala be3f1068f7921c9c8f8697577efea6580672813e samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6 samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala 0b7bcdda1639eea8239a69c31bdf42558e9077d2 samza-core/src/test/scala/org/apache/samza/storage/TestTaskStorageManager.scala 4d40f520e54beb643acd8410c772b75e2f6a9162 samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala 973ab8cfb3d248bec7efe5e338f5e667f097556d Diff: https://reviews.apache.org/r/52476/diff/ Testing --- Unit testing and manual testing has been done to verify the functionality. Thanks, Shanthoosh Venkataraman
Review Request 53002: Do not load the monitor, if the MonitorFactoryClass is not defined for the monitor in the config.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53002/ --- Review request for samza and Jake Maes. Repository: samza Description --- This patch aims to not load the monitors if the monitor factory class is not defined(set) for the monitor in the config. This will enable the users to turn on/off the monitors in samza-rest easily(just by setting the monitorFactoryClass config associated with monitor a to empty string.) Diffs - samza-rest/src/main/java/org/apache/samza/monitor/SamzaMonitorService.java ce947f7ae1175acc1ee9aa75991c726848072694 samza-rest/src/test/java/org/apache/samza/monitor/TestMonitorService.java 4618b54f5af861383df45bf7185622d36d17cd5e samza-rest/src/test/java/org/apache/samza/monitor/mock/MockMonitorFactory.java PRE-CREATION Diff: https://reviews.apache.org/r/53002/diff/ Testing --- Unit testing and manual testing are done to verify the functionality. Thanks, Shanthoosh Venkataraman
Re: Review Request 53002: Do not load the monitor, if the MonitorFactoryClass is not defined for the monitor in the config.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53002/ --- (Updated Oct. 19, 2016, 1:11 a.m.) Review request for samza and Jake Maes. Repository: samza Description --- This patch aims to not load the monitors if the monitor factory class is not defined(set) for the monitor in the config. This will enable the users to turn on/off the monitors in samza-rest easily(just by setting the monitorFactoryClass config associated with monitor a to empty string.) Diffs (updated) - build.gradle 98839f2d44569cfe04f12207117668afd593b4df samza-rest/src/main/java/org/apache/samza/monitor/SamzaMonitorService.java ce947f7ae1175acc1ee9aa75991c726848072694 samza-rest/src/test/java/org/apache/samza/monitor/TestMonitorService.java 4618b54f5af861383df45bf7185622d36d17cd5e samza-rest/src/test/java/org/apache/samza/monitor/mock/MockMonitorFactory.java PRE-CREATION Diff: https://reviews.apache.org/r/53002/diff/ Testing --- Unit testing and manual testing are done to verify the functionality. Thanks, Shanthoosh Venkataraman
Re: Review Request 53002: Do not load the monitor, if the MonitorFactoryClass is not defined for the monitor in the config.
> On Oct. 18, 2016, 11:11 p.m., Fred Ji wrote: > > samza-rest/src/main/java/org/apache/samza/monitor/SamzaMonitorService.java, > > line 71 > > <https://reviews.apache.org/r/53002/diff/1/?file=1540988#file1540988line71> > > > > nit: this line of comment is not needed since log warn is very clear. Removed. - Shanthoosh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53002/#review153208 --- On Oct. 19, 2016, 1:11 a.m., Shanthoosh Venkataraman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53002/ > --- > > (Updated Oct. 19, 2016, 1:11 a.m.) > > > Review request for samza and Jake Maes. > > > Repository: samza > > > Description > --- > > This patch aims to not load the monitors if the monitor factory class is not > defined(set) for the monitor in the config. This will enable the users to > turn on/off the monitors in samza-rest easily(just by setting the > monitorFactoryClass config associated with monitor a to empty string.) > > > Diffs > - > > build.gradle 98839f2d44569cfe04f12207117668afd593b4df > samza-rest/src/main/java/org/apache/samza/monitor/SamzaMonitorService.java > ce947f7ae1175acc1ee9aa75991c726848072694 > samza-rest/src/test/java/org/apache/samza/monitor/TestMonitorService.java > 4618b54f5af861383df45bf7185622d36d17cd5e > > samza-rest/src/test/java/org/apache/samza/monitor/mock/MockMonitorFactory.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/53002/diff/ > > > Testing > --- > > Unit testing and manual testing are done to verify the functionality. > > > Thanks, > > Shanthoosh Venkataraman > >
Re: Review Request 53002: Do not load the monitor, if the MonitorFactoryClass is not defined for the monitor in the config.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53002/ --- (Updated Oct. 19, 2016, 6 p.m.) Review request for samza and Jake Maes. Repository: samza Description (updated) --- This patch aims to not load the monitors if the monitor factory class is not defined(set) for the monitor in the config. This will enable the users to turn on/off the monitors in samza-rest easily(just by setting the monitorFactoryClass config associated with monitor a to empty string.). This is associated with the JIRA ticket : https://issues.apache.org/jira/browse/SAMZA-1039 Diffs - build.gradle 98839f2d44569cfe04f12207117668afd593b4df samza-rest/src/main/java/org/apache/samza/monitor/SamzaMonitorService.java ce947f7ae1175acc1ee9aa75991c726848072694 samza-rest/src/test/java/org/apache/samza/monitor/TestMonitorService.java 4618b54f5af861383df45bf7185622d36d17cd5e samza-rest/src/test/java/org/apache/samza/monitor/mock/MockMonitorFactory.java PRE-CREATION Diff: https://reviews.apache.org/r/53002/diff/ Testing --- Unit testing and manual testing are done to verify the functionality. Thanks, Shanthoosh Venkataraman
Re: Review Request 52476: Do not load task store which are older than delete tombstones.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52476/ --- (Updated Oct. 19, 2016, 10:04 p.m.) Review request for samza. Repository: samza Description --- Every local task store is backed up by a kafka changelog topic. Due to log compaction, delete tombstones of the changelog topic have a ttl of delete.retention.ms. Replaying the events from the changelog that has missing delete tombstones, would result in creation of an inconsistent local store(due to the missing of some delete events). This patch deletes the local stores in which difference between current time and last modified time of the offset file is greater than delete.retention.ms during the container startup. Diffs (updated) - samza-core/src/main/java/org/apache/samza/storage/StorageRecovery.java 9329edf7d724f3a0d9235354bb77936f713e3b5f samza-core/src/main/scala/org/apache/samza/config/StorageConfig.scala be3f1068f7921c9c8f8697577efea6580672813e samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6 samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala 0b7bcdda1639eea8239a69c31bdf42558e9077d2 samza-core/src/test/scala/org/apache/samza/storage/TestTaskStorageManager.scala 4d40f520e54beb643acd8410c772b75e2f6a9162 samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala 973ab8cfb3d248bec7efe5e338f5e667f097556d Diff: https://reviews.apache.org/r/52476/diff/ Testing --- Unit testing and manual testing has been done to verify the functionality. Thanks, Shanthoosh Venkataraman
Re: Review Request 52476: Do not load task store which are older than delete tombstones.
> On Oct. 17, 2016, 11:08 p.m., Prateek Maheshwari wrote: > > samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala, > > line 156 > > <https://reviews.apache.org/r/52476/diff/3/?file=1539990#file1539990line156> > > > > Unrelated, but let's make this info. Done. > On Oct. 17, 2016, 11:08 p.m., Prateek Maheshwari wrote: > > samza-core/src/main/scala/org/apache/samza/config/StorageConfig.scala, line > > 33 > > <https://reviews.apache.org/r/52476/diff/3/?file=1539988#file1539988line33> > > > > Usually more readable if you write this as a multiplication: 1 * 24 * > > 60 * 60 * 1000L Done. > On Oct. 17, 2016, 11:08 p.m., Prateek Maheshwari wrote: > > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala, > > line 532 > > <https://reviews.apache.org/r/52476/diff/3/?file=1539989#file1539989line532> > > > > Prefer passing the one config that we need explicitly instead of > > passing the config object. Moved to use changeLogDeleteRetentions map. > On Oct. 17, 2016, 11:08 p.m., Prateek Maheshwari wrote: > > samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala, > > line 29 > > <https://reviews.apache.org/r/52476/diff/3/?file=1539990#file1539990line29> > > > > Unrelated to RB but prefer explicit imports. Done. > On Oct. 17, 2016, 11:08 p.m., Prateek Maheshwari wrote: > > samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala, > > line 26 > > <https://reviews.apache.org/r/52476/diff/3/?file=1539990#file1539990line26> > > > > Delete or import explicitly. Done. > On Oct. 17, 2016, 11:08 p.m., Prateek Maheshwari wrote: > > samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala, > > line 107 > > <https://reviews.apache.org/r/52476/diff/3/?file=1539990#file1539990line107> > > > > Add method description. Done. > On Oct. 17, 2016, 11:08 p.m., Prateek Maheshwari wrote: > > samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala, > > line 114 > > <https://reviews.apache.org/r/52476/diff/3/?file=1539990#file1539990line114> > > > > Another case we ran into on Friday - if the oldest offset in the > > changelog topic is newer than the offset in the OFFSET file. Do you need to > > handle that here? > > > > Nitpick: would isStaleStore be clearer? Discussed offline. This is a regular scenario that happens with compaction (message expiration in general w.r.t topics). When the offset in the offset file is older than oldest offset in changelog, it indicates that compaction has happened. To not miss messages from the topic in the consumption, users have to consume from the oldest offset in the changelog, which is controlled by the config parameter systems.name.consumer.auto.offset.reset. > On Oct. 17, 2016, 11:08 p.m., Prateek Maheshwari wrote: > > samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala, > > line 122 > > <https://reviews.apache.org/r/52476/diff/3/?file=1539990#file1539990line122> > > > > Mention somewhere in the message that this means that the store is > > stale. Done. - Shanthoosh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52476/#review152976 --- On Oct. 19, 2016, 10:04 p.m., Shanthoosh Venkataraman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52476/ > --- > > (Updated Oct. 19, 2016, 10:04 p.m.) > > > Review request for samza. > > > Repository: samza > > > Description > --- > > Every local task store is backed up by a kafka changelog topic. Due to log > compaction, delete tombstones of the changelog topic have a ttl of > delete.retention.ms. Replaying the events from the changelog that has missing > delete tombstones, would result in creation of an inconsistent local > store(due to the missing of some delete events). This patch deletes the local > stores in which difference between current time and last modified time of the > offset file is greater than delete.retention.ms during the container startup. > > > Diffs > - > > samza-core/src/main/java/org/apache/samza/storage/StorageRecovery.java > 9329edf7d724f3a0d9235354bb77936f713e3b5f > samza-core/src/main/scala/org/apach
Re: Review Request 52492: Adding monitor to clean up stale local stores of jobs/tasks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52492/ --- (Updated Oct. 20, 2016, 9:52 p.m.) Review request for samza. Repository: samza Description --- This patch contains the samza-rest monitor that periodically cleans up the stale local stores of dead jobs/tasks. It performs the store deletion in two phases. Initially it deletes the offset file in the local task stores if the following condition is true. ((jobIsNotRunning || preferedHost != nmHost) && offsetFilelastModifiedTime is greater than deleteRetention). During the subsequent run, it deletes the local task stores if it does not contain offset file. Please refer to the design doc of SAMZA-656 (https://issues.apache.org/jira/secure/attachment/12828083/DESIGN-SAMZA-656.pdf) for more details. Diffs (updated) - build.gradle 2bea27b75288d3103178bc3762b9556f6e69cdd1 samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitorConfig.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitorFactory.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/ResourceConstants.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/monitor/TestLocalStoreMonitor.java PRE-CREATION Diff: https://reviews.apache.org/r/52492/diff/ Testing --- Unit testing and manual testing are done to verify the functionality. Thanks, Shanthoosh Venkataraman
Re: Review Request 52492: Adding monitor to clean up stale local stores of jobs/tasks.
> On Oct. 17, 2016, 11:22 p.m., Jake Maes wrote: > > samza-rest/src/main/java/org/apache/samza/monitor/YarnLocalStoreMonitor.java, > > line 107 > > <https://reviews.apache.org/r/52492/diff/3-4/?file=1530145#file1530145line107> > > > > nit: I think this would read better if demorgans law were applied Done. - Shanthoosh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52492/#review152981 --- On Oct. 20, 2016, 9:52 p.m., Shanthoosh Venkataraman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52492/ > --- > > (Updated Oct. 20, 2016, 9:52 p.m.) > > > Review request for samza. > > > Repository: samza > > > Description > --- > > This patch contains the samza-rest monitor that periodically cleans up the > stale local stores of dead jobs/tasks. It performs the store deletion in two > phases. Initially it deletes the offset file in the local task stores if the > following condition is true. ((jobIsNotRunning || preferedHost != nmHost) && > offsetFilelastModifiedTime is greater than deleteRetention). During the > subsequent run, it deletes the local task stores if it does not contain > offset file. Please refer to the design doc of SAMZA-656 > (https://issues.apache.org/jira/secure/attachment/12828083/DESIGN-SAMZA-656.pdf) > for more details. > > > Diffs > - > > build.gradle 2bea27b75288d3103178bc3762b9556f6e69cdd1 > samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitorConfig.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitorFactory.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/resources/ResourceConstants.java > PRE-CREATION > > samza-rest/src/test/java/org/apache/samza/monitor/TestLocalStoreMonitor.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/52492/diff/ > > > Testing > --- > > Unit testing and manual testing are done to verify the functionality. > > > Thanks, > > Shanthoosh Venkataraman > >
Re: Review Request 52476: Do not load task store which are older than delete tombstones.
> On Oct. 20, 2016, 11:46 p.m., Boris Shkolnik wrote: > > samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala, > > line 132 > > <https://reviews.apache.org/r/52476/diff/4/?file=1541857#file1541857line132> > > > > nit. using System.currentTimeMillis directly makes it difficult to unit > > test. Done. Injected SystemClock dependency as a parameter. - Shanthoosh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52476/#review153468 ------- On Oct. 22, 2016, 10:06 p.m., Shanthoosh Venkataraman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52476/ > --- > > (Updated Oct. 22, 2016, 10:06 p.m.) > > > Review request for samza. > > > Repository: samza > > > Description > --- > > Every local task store is backed up by a kafka changelog topic. Due to log > compaction, delete tombstones of the changelog topic have a ttl of > delete.retention.ms. Replaying the events from the changelog that has missing > delete tombstones, would result in creation of an inconsistent local > store(due to the missing of some delete events). This patch deletes the local > stores in which difference between current time and last modified time of the > offset file is greater than delete.retention.ms during the container startup. > > > Diffs > - > > samza-core/src/main/java/org/apache/samza/storage/StorageRecovery.java > 9329edf7d724f3a0d9235354bb77936f713e3b5f > samza-core/src/main/scala/org/apache/samza/config/StorageConfig.scala > be3f1068f7921c9c8f8697577efea6580672813e > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > 05a996c98075ea8ed3767af666b9beeb1933f2a6 > samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala > 0b7bcdda1639eea8239a69c31bdf42558e9077d2 > > samza-core/src/test/scala/org/apache/samza/storage/TestTaskStorageManager.scala > 4d40f520e54beb643acd8410c772b75e2f6a9162 > samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala > 973ab8cfb3d248bec7efe5e338f5e667f097556d > > Diff: https://reviews.apache.org/r/52476/diff/ > > > Testing > --- > > Unit testing and manual testing has been done to verify the functionality. > > > Thanks, > > Shanthoosh Venkataraman > >
Re: Review Request 52476: Do not load task store which are older than delete tombstones.
> On Oct. 21, 2016, 12:45 a.m., Prateek Maheshwari wrote: > > samza-core/src/main/java/org/apache/samza/storage/StorageRecovery.java, > > line 254 > > <https://reviews.apache.org/r/52476/diff/4/?file=1541854#file1541854line254> > > > > Does jobConfig.getChangeLog...() (implicit conversion) not work? No, the implicit conversion doesn't work here. The convenience method is part of StorageConfig class. > On Oct. 21, 2016, 12:45 a.m., Prateek Maheshwari wrote: > > samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala, > > line 130 > > <https://reviews.apache.org/r/52476/diff/4/?file=1541857#file1541857line130> > > > > First %s should be store name. Add another %s at the end for > > loggedStoreDir. This log message belongs to the task store, which would in itself contain the store name. Adding store name here is unnecessary. > On Oct. 21, 2016, 12:45 a.m., Prateek Maheshwari wrote: > > samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala, line > > 144 > > <https://reviews.apache.org/r/52476/diff/4/?file=1541859#file1541859line144> > > > > implicit conversion should probably work. No, implicit conversion doesn't work here, that is the reason for creating the object explicitly. > On Oct. 21, 2016, 12:45 a.m., Prateek Maheshwari wrote: > > samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala, > > line 186 > > <https://reviews.apache.org/r/52476/diff/4/?file=1541857#file1541857line186> > > > > s/partition/logged storage partition to be consistent with next message. Done. > On Oct. 21, 2016, 12:45 a.m., Prateek Maheshwari wrote: > > samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala, > > line 133 > > <https://reviews.apache.org/r/52476/diff/4/?file=1541857#file1541857line133> > > > > Log both last modified time and delete retention ms values too. Done. > On Oct. 21, 2016, 12:45 a.m., Prateek Maheshwari wrote: > > samza-core/src/main/scala/org/apache/samza/config/StorageConfig.scala, line > > 57 > > <https://reviews.apache.org/r/52476/diff/4/?file=1541855#file1541855line57> > > > > getChangeLogDeleteRetentionsInMs Done. - Shanthoosh ------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52476/#review153478 --- On Oct. 22, 2016, 10:06 p.m., Shanthoosh Venkataraman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52476/ > --- > > (Updated Oct. 22, 2016, 10:06 p.m.) > > > Review request for samza. > > > Repository: samza > > > Description > --- > > Every local task store is backed up by a kafka changelog topic. Due to log > compaction, delete tombstones of the changelog topic have a ttl of > delete.retention.ms. Replaying the events from the changelog that has missing > delete tombstones, would result in creation of an inconsistent local > store(due to the missing of some delete events). This patch deletes the local > stores in which difference between current time and last modified time of the > offset file is greater than delete.retention.ms during the container startup. > > > Diffs > - > > samza-core/src/main/java/org/apache/samza/storage/StorageRecovery.java > 9329edf7d724f3a0d9235354bb77936f713e3b5f > samza-core/src/main/scala/org/apache/samza/config/StorageConfig.scala > be3f1068f7921c9c8f8697577efea6580672813e > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > 05a996c98075ea8ed3767af666b9beeb1933f2a6 > samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala > 0b7bcdda1639eea8239a69c31bdf42558e9077d2 > > samza-core/src/test/scala/org/apache/samza/storage/TestTaskStorageManager.scala > 4d40f520e54beb643acd8410c772b75e2f6a9162 > samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala > 973ab8cfb3d248bec7efe5e338f5e667f097556d > > Diff: https://reviews.apache.org/r/52476/diff/ > > > Testing > --- > > Unit testing and manual testing has been done to verify the functionality. > > > Thanks, > > Shanthoosh Venkataraman > >
Re: Review Request 52476: Do not load task store which are older than delete tombstones.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52476/ --- (Updated Oct. 22, 2016, 10:06 p.m.) Review request for samza. Repository: samza Description --- Every local task store is backed up by a kafka changelog topic. Due to log compaction, delete tombstones of the changelog topic have a ttl of delete.retention.ms. Replaying the events from the changelog that has missing delete tombstones, would result in creation of an inconsistent local store(due to the missing of some delete events). This patch deletes the local stores in which difference between current time and last modified time of the offset file is greater than delete.retention.ms during the container startup. Diffs (updated) - samza-core/src/main/java/org/apache/samza/storage/StorageRecovery.java 9329edf7d724f3a0d9235354bb77936f713e3b5f samza-core/src/main/scala/org/apache/samza/config/StorageConfig.scala be3f1068f7921c9c8f8697577efea6580672813e samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6 samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala 0b7bcdda1639eea8239a69c31bdf42558e9077d2 samza-core/src/test/scala/org/apache/samza/storage/TestTaskStorageManager.scala 4d40f520e54beb643acd8410c772b75e2f6a9162 samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala 973ab8cfb3d248bec7efe5e338f5e667f097556d Diff: https://reviews.apache.org/r/52476/diff/ Testing --- Unit testing and manual testing has been done to verify the functionality. Thanks, Shanthoosh Venkataraman
Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job
> On Oct. 17, 2016, 9:38 p.m., Jake Maes wrote: > > docs/learn/documentation/versioned/rest/resources/tasks.md, line 105 > > <https://reviews.apache.org/r/52168/diff/6/?file=1535245#file1535245line105> > > > > This comment still looks out of place. Maybe it should just be > > incorporated in the table. It's present already as a part of the configuration table. First configuration `task.proxy.factory.class` is added with description and required field. I just added it above to re-instate that importance of that config. > On Oct. 17, 2016, 9:38 p.m., Jake Maes wrote: > > samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java, > > line 56 > > <https://reviews.apache.org/r/52168/diff/6/?file=1535268#file1535268line56> > > > > Why has this property been added to this test? It should not be > > mandatory. If configuration for the list of resource factories is undefined, then it is defaulted to DefaultResourceFactory which loads all the resources. Here we would just want to load the JobsResource, hence I'm explicitly loading it with custom resource factory. - Shanthoosh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52168/#review152928 --- On Oct. 13, 2016, 11:57 p.m., Shanthoosh Venkataraman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52168/ > --- > > (Updated Oct. 13, 2016, 11:57 p.m.) > > > Review request for samza. > > > Repository: samza > > > Description > --- > > This patch contains the following changes > * Http get api to list the complete details of all the tasks that belongs to > a job. > * Refactored some methods in coordinator stream, to reuse the existing > functionality of getting jobConfig from the coordinator stream. > > > Diffs > - > > docs/learn/documentation/versioned/rest/resource-directory.md > 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef > docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION > samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala > 13b72fae7815ddaea7ae03a24f1a426ca51613cc > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > 05a996c98075ea8ed3767af666b9beeb1933f2a6 > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala > df63b97e9d598ecd1840111ba490a723e410d089 > samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala > 022b480856483059cb9f837a08f97a718bc04c31 > samza-core/src/main/scala/org/apache/samza/util/Util.scala > c4836f202f7eda1d4e71eac94fd48e46207b0316 > samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java > PRE-CREATION > samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java > 4d8647f3e1e650632e38b47ba5a8a2dac004f545 > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java > 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java > a935c98730f85f448c688a6baf2e8ddffdbb2cb4 > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java > 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java > PRE-CREATION > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java > e0224c6bcf4aeaa336e5786ac472482507fcd382 > samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java > a566db598c284d69ea61af88fdc0851483d5a089 > > samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java > 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797 > samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java > P
Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52168/ --- (Updated Oct. 24, 2016, 10 p.m.) Review request for samza. Repository: samza Description --- This patch contains the following changes * Http get api to list the complete details of all the tasks that belongs to a job. * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream. Diffs (updated) - docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala 13b72fae7815ddaea7ae03a24f1a426ca51613cc samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6 samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089 samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31 samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316 samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545 samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4 samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/ConfigFactoryBuilder.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382 samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089 samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797 samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION Diff: https://reviews.apache.org/r/52168/diff/ Testing --- Manual and unit testing has been done to verify the apis. Thanks, Shanthoosh Venkataraman
Re: Review Request 52492: Adding monitor to clean up stale local stores of jobs/tasks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52492/ --- (Updated Oct. 25, 2016, 8:14 p.m.) Review request for samza. Repository: samza Description --- This patch contains the samza-rest monitor that periodically cleans up the stale local stores of dead jobs/tasks. It performs the store deletion in two phases. Initially it deletes the offset file in the local task stores if the following condition is true. ((jobIsNotRunning || preferedHost != nmHost) && offsetFilelastModifiedTime is greater than deleteRetention). During the subsequent run, it deletes the local task stores if it does not contain offset file. Please refer to the design doc of SAMZA-656 (https://issues.apache.org/jira/secure/attachment/12828083/DESIGN-SAMZA-656.pdf) for more details. Diffs (updated) - build.gradle 2bea27b75288d3103178bc3762b9556f6e69cdd1 samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitorConfig.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitorFactory.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/ResourceConstants.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/monitor/TestLocalStoreMonitor.java PRE-CREATION Diff: https://reviews.apache.org/r/52492/diff/ Testing --- Unit testing and manual testing are done to verify the functionality. Thanks, Shanthoosh Venkataraman
Re: Review Request 52492: Adding monitor to clean up stale local stores of jobs/tasks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52492/ --- (Updated Oct. 26, 2016, 12:41 a.m.) Review request for samza. Repository: samza Description --- This patch contains the samza-rest monitor that periodically cleans up the stale local stores of dead jobs/tasks. It performs the store deletion in two phases. Initially it deletes the offset file in the local task stores if the following condition is true. ((jobIsNotRunning || preferedHost != nmHost) && offsetFilelastModifiedTime is greater than deleteRetention). During the subsequent run, it deletes the local task stores if it does not contain offset file. Please refer to the design doc of SAMZA-656 (https://issues.apache.org/jira/secure/attachment/12828083/DESIGN-SAMZA-656.pdf) for more details. Diffs (updated) - build.gradle 2bea27b75288d3103178bc3762b9556f6e69cdd1 samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitorConfig.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitorFactory.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/ResourceConstants.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/monitor/TestLocalStoreMonitor.java PRE-CREATION Diff: https://reviews.apache.org/r/52492/diff/ Testing --- Unit testing and manual testing are done to verify the functionality. Thanks, Shanthoosh Venkataraman
Re: Review Request 52492: Adding monitor to clean up stale local stores of jobs/tasks.
> On Oct. 25, 2016, 9:31 p.m., Jagadish Venkatraman wrote: > > samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java, > > line 166 > > <https://reviews.apache.org/r/52492/diff/5/?file=1542707#file1542707line166> > > > > JobsClient seems to be a generic class and not really tied to any > > LocalStoreMonitor. Furthermore, we pass in an instance of the client to the > > monitor. > > > > For example, any other monitor that wants to make retriable requests > > could potentially use this. Can this be a separate class? There is no other monitor requiring this currently. It would make sense to move this outside, once such a need arises. - Shanthoosh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52492/#review153772 ------- On Oct. 26, 2016, 12:41 a.m., Shanthoosh Venkataraman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52492/ > --- > > (Updated Oct. 26, 2016, 12:41 a.m.) > > > Review request for samza. > > > Repository: samza > > > Description > --- > > This patch contains the samza-rest monitor that periodically cleans up the > stale local stores of dead jobs/tasks. It performs the store deletion in two > phases. Initially it deletes the offset file in the local task stores if the > following condition is true. ((jobIsNotRunning || preferedHost != nmHost) && > offsetFilelastModifiedTime is greater than deleteRetention). During the > subsequent run, it deletes the local task stores if it does not contain > offset file. Please refer to the design doc of SAMZA-656 > (https://issues.apache.org/jira/secure/attachment/12828083/DESIGN-SAMZA-656.pdf) > for more details. > > > Diffs > - > > build.gradle 2bea27b75288d3103178bc3762b9556f6e69cdd1 > samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitorConfig.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitorFactory.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/resources/ResourceConstants.java > PRE-CREATION > > samza-rest/src/test/java/org/apache/samza/monitor/TestLocalStoreMonitor.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/52492/diff/ > > > Testing > --- > > Unit testing and manual testing are done to verify the functionality. > > > Thanks, > > Shanthoosh Venkataraman > >
Review Request 53297: Initial version of adding metrics into samza rest.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53297/ --- Review request for samza. Repository: samza Description --- This patch aims at enabling users to define custom reporters to send metrics from the monitors. Configurations required for the definition of the metrics reporters follows the same convention as of the samza jobs. Diffs - docs/learn/documentation/versioned/rest/monitors.md 46678bbe5fed99f767c3324dc9578ee1a64cec66 samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala e0468ee89c89fd720834461771ebb36475475bcb samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala f24beb1e099dd44b15b475e0a4a7f70560c6965e samza-core/src/main/scala/org/apache/samza/util/MetricsReporterLoader.scala PRE-CREATION samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316 samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java 47b0663637f6db187d86961377ee3ee203b73fdb samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java 2a3e83a24a5343bb53b93fc9d0a647c1b253714b samza-rest/src/test/java/org/apache/samza/rest/resources/TestSamzaRestService.java PRE-CREATION samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala 8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca Diff: https://reviews.apache.org/r/53297/diff/ Testing --- Unit tests are done to verify the intended functionality. Thanks, Shanthoosh Venkataraman
Re: Review Request 52492: Adding monitor to clean up stale local stores of jobs/tasks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52492/ --- (Updated Oct. 31, 2016, 11:04 p.m.) Review request for samza. Repository: samza Description --- This patch contains the samza-rest monitor that periodically cleans up the stale local stores of dead jobs/tasks. It performs the store deletion in two phases. Initially it deletes the offset file in the local task stores if the following condition is true. ((jobIsNotRunning || preferedHost != nmHost) && offsetFilelastModifiedTime is greater than deleteRetention). During the subsequent run, it deletes the local task stores if it does not contain offset file. Please refer to the design doc of SAMZA-656 (https://issues.apache.org/jira/secure/attachment/12828083/DESIGN-SAMZA-656.pdf) for more details. Diffs (updated) - build.gradle 2bea27b75288d3103178bc3762b9556f6e69cdd1 samza-rest/src/main/java/org/apache/samza/monitor/JobsClient.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitorConfig.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitorFactory.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/ResourceConstants.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/monitor/TestLocalStoreMonitor.java PRE-CREATION Diff: https://reviews.apache.org/r/52492/diff/ Testing --- Unit testing and manual testing are done to verify the functionality. Thanks, Shanthoosh Venkataraman
Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52168/ --- (Updated Nov. 3, 2016, 12:58 a.m.) Review request for samza. Repository: samza Description --- This patch contains the following changes * Http get api to list the complete details of all the tasks that belongs to a job. * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream. Diffs (updated) - docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala 13b72fae7815ddaea7ae03a24f1a426ca51613cc samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6 samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089 samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31 samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316 samza-core/src/test/scala/org/apache/samza/coordinator/TestJobCoordinator.scala fcabc69a829fd26b7f4e422d9877ec0364d308ce samza-rest/src/main/config/samza-rest.properties 7be0b47d1466d2199ae278247e8d81522fb6a91c samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545 samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4 samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382 samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089 samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797 samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION Diff: https://reviews.apache.org/r/52168/diff/ Testing --- Manual and unit testing has been done to verify the apis. Thanks, Shanthoosh Venkataraman
Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job
> On Oct. 25, 2016, 9:06 p.m., Navina Ramesh wrote: > > samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala, line 47 > > <https://reviews.apache.org/r/52168/diff/7/?file=1544665#file1544665line47> > > > > I would think the ideal place for this method will be in Util as > > opposed to JobConfig. Can you please elaborate the rationale behind your > > choice? It was initially added as static method in Util. However, util contains methods that does http get reqeusts, removing files, creating files, static factory methods. It looks like a place holder to hold all the things. Hence added it over here. May be into a seperate class. - Shanthoosh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52168/#review153819 ------- On Nov. 3, 2016, 12:58 a.m., Shanthoosh Venkataraman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52168/ > --- > > (Updated Nov. 3, 2016, 12:58 a.m.) > > > Review request for samza. > > > Repository: samza > > > Description > --- > > This patch contains the following changes > * Http get api to list the complete details of all the tasks that belongs to > a job. > * Refactored some methods in coordinator stream, to reuse the existing > functionality of getting jobConfig from the coordinator stream. > > > Diffs > - > > docs/learn/documentation/versioned/rest/resource-directory.md > 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef > docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION > samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala > 13b72fae7815ddaea7ae03a24f1a426ca51613cc > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > 05a996c98075ea8ed3767af666b9beeb1933f2a6 > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala > df63b97e9d598ecd1840111ba490a723e410d089 > samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala > 022b480856483059cb9f837a08f97a718bc04c31 > samza-core/src/main/scala/org/apache/samza/util/Util.scala > c4836f202f7eda1d4e71eac94fd48e46207b0316 > > samza-core/src/test/scala/org/apache/samza/coordinator/TestJobCoordinator.scala > fcabc69a829fd26b7f4e422d9877ec0364d308ce > samza-rest/src/main/config/samza-rest.properties > 7be0b47d1466d2199ae278247e8d81522fb6a91c > samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java > PRE-CREATION > samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java > 4d8647f3e1e650632e38b47ba5a8a2dac004f545 > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java > 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java > a935c98730f85f448c688a6baf2e8ddffdbb2cb4 > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java > 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java > PRE-CREATION > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java > e0224c6bcf4aeaa336e5786ac472482507fcd382 > samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java > a566db598c284d69ea61af88fdc0851483d5a089 > > samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java > 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797 > samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java > PRE-CREATION > samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java > PRE-CREATION > > samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java > 7db437b348ecd286185898b8f8ab0220d59da71a >
Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52168/ --- (Updated Nov. 3, 2016, 11:19 p.m.) Review request for samza. Repository: samza Description --- This patch contains the following changes * Http get api to list the complete details of all the tasks that belongs to a job. * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream. Diffs (updated) - docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala 13b72fae7815ddaea7ae03a24f1a426ca51613cc samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6 samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089 samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31 samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316 samza-core/src/test/scala/org/apache/samza/coordinator/TestJobCoordinator.scala fcabc69a829fd26b7f4e422d9877ec0364d308ce samza-rest/src/main/config/samza-rest.properties 7be0b47d1466d2199ae278247e8d81522fb6a91c samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545 samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4 samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382 samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089 samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797 samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION Diff: https://reviews.apache.org/r/52168/diff/ Testing --- Manual and unit testing has been done to verify the apis. Thanks, Shanthoosh Venkataraman
Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52168/ --- (Updated Nov. 4, 2016, 1:04 a.m.) Review request for samza. Repository: samza Description --- This patch contains the following changes * Http get api to list the complete details of all the tasks that belongs to a job. * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream. Diffs (updated) - docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala 13b72fae7815ddaea7ae03a24f1a426ca51613cc samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6 samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089 samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31 samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316 samza-core/src/test/scala/org/apache/samza/coordinator/TestJobCoordinator.scala fcabc69a829fd26b7f4e422d9877ec0364d308ce samza-rest/src/main/config/samza-rest.properties 7be0b47d1466d2199ae278247e8d81522fb6a91c samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545 samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4 samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382 samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089 samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797 samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION Diff: https://reviews.apache.org/r/52168/diff/ Testing --- Manual and unit testing has been done to verify the apis. Thanks, Shanthoosh Venkataraman
Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job
samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java, > > line 40 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552739#file1552739line40> > > > > Is the CONFIG_ prefix a samza-rest convention? Yes, it's the convention in samza-rest. > On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote: > > samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java, line 27 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552728#file1552728line27> > > > > "partition id" Done. > On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote: > > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, > > line 309 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552723#file1552723line309> > > > > Unrelated to RB: mutable.Map instead of util.HashMap? I would really like to make this change, however, in this class, everywhere util.Map, util.Set has been used extensively. If i just change here alone to collections.mutable.mao, it will look incoherent and changing at every other place would be a harder effort. > On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote: > > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, > > lines 110-123 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552723#file1552723line110> > > > > Can't tell in RB: could this be replaced by retrieveJobModel()? There are lot of state changes that has to be done after retrieveJobModel. LocalityManager, changeLogManager are required for that. This refactoring would make it unclean. > On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote: > > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, > > line 98 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552723#file1552723line98> > > > > Extract val for metadata cache, systemAdmins. Done. > On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote: > > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, > > lines 99-102 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552723#file1552723line99> > > > > Not required for the other place they're used? Yes. > On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote: > > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, > > line 340 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552723#file1552723line340> > > > > Previous line. Done. > On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote: > > docs/learn/documentation/versioned/rest/resources/tasks.md, line 22 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552720#file1552720line22> > > > > "support operations" Done. > On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote: > > docs/learn/documentation/versioned/rest/resources/tasks.md, line 25 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552720#file1552720line25> > > > > "with their" Done. - Shanthoosh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52168/#review154675 --- On Nov. 4, 2016, 1:04 a.m., Shanthoosh Venkataraman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52168/ > --- > > (Updated Nov. 4, 2016, 1:04 a.m.) > > > Review request for samza. > > > Repository: samza > > > Description > --- > > This patch contains the following changes > * Http get api to list the complete details of all the tasks that belongs to > a job. > * Refactored some methods in coordinator stream, to reuse the existing > functionality of getting jobConfig from the coordinator stream. > > > Diffs > - > > docs/learn/documentation/versioned/rest/resource-directory.md > 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef > docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION > samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala > 13b72fae7815ddaea7ae03a24f1a426ca51613cc > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > 05a996c98075ea8ed3767af666b9beeb1933f2a6 > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala > df63b97e9d598ecd1840111ba490a723e410d089 > samza-core/src/main/scala/org/apache/samza/job/J
Re: Review Request 53297: Initial version of adding metrics into samza rest.
> On Nov. 5, 2016, 6:47 p.m., Jake Maes wrote: > > samza-core/src/main/scala/org/apache/samza/util/Util.scala, line 336 > > <https://reviews.apache.org/r/53297/diff/1/?file=1549405#file1549405line336> > > > > This edit looks like a mistake. > > > > Did this file need to be modified at all? Idea was showing error because of that tag. It's not required for this patch. Removed. > On Nov. 5, 2016, 6:47 p.m., Jake Maes wrote: > > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java, line > > 73 > > <https://reviews.apache.org/r/53297/diff/1/?file=1549407#file1549407line73> > > > > What's the purpose of this static factory? > > > > The typical reason is to construct the object differently (e.g. a > > different subclass) based on some parameter. > > > > I don't see any value of the approach here. > > > > If there is some value, then the constructor should be made private. Discussed offline. It was harder to test SamzaRestService with all new operator instantiations happening in the constructor. Hence, mocking out these dependencies were harder. Also with this approach, looking at the constructor we get to know the dependencies of SamzaRestService explicitly. However, since there are no use cases which would require this factory, removing this factory. Keeping the constructor as it is, moving the instantiation into the main method. > On Nov. 5, 2016, 6:47 p.m., Jake Maes wrote: > > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java, line > > 75 > > <https://reviews.apache.org/r/53297/diff/1/?file=1549407#file1549407line75> > > > > I don't think the MetricsConfig constructure takes a subset. > > > > I think it takes the root and expects to find the "metrics" prefix > > Jake Maes wrote: > s/constructure/constructor > > phonetic brain fail Yes, that's true. It expects the root and finds the metrics prefix. Hence, stripPrefix is passed on as false, so that prefix isn't removed. This just selects the config subset that starts with METRICS_PREFIX, without removing the prefix. The goal is to not pass on the entire config object and pass only metrics related config into MetricsConfig constructor. > On Nov. 5, 2016, 6:47 p.m., Jake Maes wrote: > > samza-core/src/main/scala/org/apache/samza/util/MetricsReporterLoader.scala, > > line 40 > > <https://reviews.apache.org/r/53297/diff/1/?file=1549404#file1549404line40> > > > > We're moving away from Scala. All new files should be Java. Done. Migrated from scala to java. > On Nov. 5, 2016, 6:47 p.m., Jake Maes wrote: > > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java, line > > 60 > > <https://reviews.apache.org/r/53297/diff/1/?file=1549407#file1549407line60> > > > > The Resources will eventually emit metrics too, so I think this value > > is too specific. Changed it to SamzaRest. Not sure of the implications of using a proper name here. For instance, in SamzaContainerMetrics source string is assigned to value "unknown". I'm most certain that this string is a placeholder to register MetricsRegistry instances with MetricsReporter and not used when reporting the actual metrics. - Shanthoosh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53297/#review155061 --- On Nov. 8, 2016, 11:13 p.m., Shanthoosh Venkataraman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53297/ > --- > > (Updated Nov. 8, 2016, 11:13 p.m.) > > > Review request for samza and Jake Maes. > > > Repository: samza > > > Description > --- > > This patch aims at enabling users to define custom reporters to send metrics > from the monitors. Configurations required for the definition of the metrics > reporters follows the same convention as of the samza jobs. > > > Diffs > - > > docs/learn/documentation/versioned/rest/monitors.md > 46678bbe5fed99f767c3324dc9578ee1a64cec66 > samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java > PRE-CREATION > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > e0468ee89c89fd720834461771ebb36475475bcb > > samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManage
Re: Review Request 53297: Initial version of adding metrics into samza rest.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53297/ --- (Updated Nov. 8, 2016, 11:13 p.m.) Review request for samza and Jake Maes. Repository: samza Description --- This patch aims at enabling users to define custom reporters to send metrics from the monitors. Configurations required for the definition of the metrics reporters follows the same convention as of the samza jobs. Diffs (updated) - docs/learn/documentation/versioned/rest/monitors.md 46678bbe5fed99f767c3324dc9578ee1a64cec66 samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java PRE-CREATION samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala e0468ee89c89fd720834461771ebb36475475bcb samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala f24beb1e099dd44b15b475e0a4a7f70560c6965e samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java 47b0663637f6db187d86961377ee3ee203b73fdb samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java 2a3e83a24a5343bb53b93fc9d0a647c1b253714b samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java PRE-CREATION samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala 8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca Diff: https://reviews.apache.org/r/53297/diff/ Testing --- Unit tests are done to verify the intended functionality. Thanks, Shanthoosh Venkataraman
Re: Review Request 53297: Initial version of adding metrics into samza rest.
- Shanthoosh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53297/#review155061 --- On Nov. 8, 2016, 11:13 p.m., Shanthoosh Venkataraman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53297/ > --- > > (Updated Nov. 8, 2016, 11:13 p.m.) > > > Review request for samza and Jake Maes. > > > Repository: samza > > > Description > --- > > This patch aims at enabling users to define custom reporters to send metrics > from the monitors. Configurations required for the definition of the metrics > reporters follows the same convention as of the samza jobs. > > > Diffs > - > > docs/learn/documentation/versioned/rest/monitors.md > 46678bbe5fed99f767c3324dc9578ee1a64cec66 > samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java > PRE-CREATION > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > e0468ee89c89fd720834461771ebb36475475bcb > > samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala > f24beb1e099dd44b15b475e0a4a7f70560c6965e > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java > 47b0663637f6db187d86961377ee3ee203b73fdb > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java > 2a3e83a24a5343bb53b93fc9d0a647c1b253714b > samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java > PRE-CREATION > > samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala > 8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca > > Diff: https://reviews.apache.org/r/53297/diff/ > > > Testing > --- > > Unit tests are done to verify the intended functionality. > > > Thanks, > > Shanthoosh Venkataraman > >
Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52168/ --- (Updated Nov. 9, 2016, 1:23 a.m.) Review request for samza. Repository: samza Description --- This patch contains the following changes * Http get api to list the complete details of all the tasks that belongs to a job. * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream. Diffs (updated) - docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089 samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31 samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316 samza-rest/src/main/config/samza-rest.properties 7be0b47d1466d2199ae278247e8d81522fb6a91c samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545 samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4 samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382 samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089 samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797 samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION Diff: https://reviews.apache.org/r/52168/diff/ Testing --- Manual and unit testing has been done to verify the apis. Thanks, Shanthoosh Venkataraman
Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job
> On Nov. 4, 2016, 8:52 p.m., Jagadish Venkatraman wrote: > > docs/learn/documentation/versioned/rest/resources/tasks.md, line 103 > > <https://reviews.apache.org/r/52168/diff/10/?file=1553937#file1553937line103> > > > > Why do we have preferredHost here? The terminology is really confusing. > > Isn't `preferredHost` referring to container's host usually? Was the result of careless refactoring. Removed. > On Nov. 4, 2016, 8:52 p.m., Jagadish Venkatraman wrote: > > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala, > > line 144 > > <https://reviews.apache.org/r/52168/diff/10/?file=1553939#file1553939line144> > > > > Nuke this method. > > > > Doesn't add any value apart from string concat. Curious about what > > value this adds? Removed container name from the TasksResource. This is no longer needed. Removed. > On Nov. 4, 2016, 8:52 p.m., Jagadish Venkatraman wrote: > > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, > > line 316 > > <https://reviews.apache.org/r/52168/diff/10/?file=1553940#file1553940line316> > > > > Reword docs. > > > > I'm sure there are other ways to read changelog partition mapping apart > > from this method. Done. > On Nov. 4, 2016, 8:52 p.m., Jagadish Venkatraman wrote: > > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, > > line 320 > > <https://reviews.apache.org/r/52168/diff/10/?file=1553940#file1553940line320> > > > > Not convinced we need this utility method here. > > > > Also, read seems to call `start` on the changeLog manager while `write` > > does not. > > > > Then, is the assumption that read must be called prior to write.? This method is inlined to its invocation. > On Nov. 4, 2016, 8:52 p.m., Jagadish Venkatraman wrote: > > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, > > line 337 > > <https://reviews.apache.org/r/52168/diff/10/?file=1553940#file1553940line337> > > > > This method does not add value. > > > > Also, it's weird that `writeChangeLogMapping` is doing a > > `Read-modify-write operation` on the manager. This method is inlined to its invocation. > On Nov. 4, 2016, 8:52 p.m., Jagadish Venkatraman wrote: > > docs/learn/documentation/versioned/rest/resources/tasks.md, line 23 > > <https://reviews.apache.org/r/52168/diff/10/?file=1553937#file1553937line23> > > > > Can we link this to the JobsResource? That way, it;s obvious to the > > reader? Done. > On Nov. 4, 2016, 8:52 p.m., Jagadish Venkatraman wrote: > > docs/learn/documentation/versioned/rest/resources/tasks.md, line 22 > > <https://reviews.apache.org/r/52168/diff/10/?file=1553937#file1553937line22> > > > > I don't think the line that `Additional operations will be added later` > > is meaningful. You can maybe remove it? Done. - Shanthoosh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52168/#review154972 --- On Nov. 9, 2016, 1:23 a.m., Shanthoosh Venkataraman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52168/ > --- > > (Updated Nov. 9, 2016, 1:23 a.m.) > > > Review request for samza. > > > Repository: samza > > > Description > --- > > This patch contains the following changes > * Http get api to list the complete details of all the tasks that belongs to > a job. > * Refactored some methods in coordinator stream, to reuse the existing > functionality of getting jobConfig from the coordinator stream. > > > Diffs > - > > docs/learn/documentation/versioned/rest/resource-directory.md > 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef > docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala > df63b97e9d598ecd1840111ba490a723e410d089 > samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala > 022b480856483059cb9f837a08f97a718bc04c31 > samza-core/src/main/scala/org/apache/samza/util/Util.scala > c4836f202f7eda1d4e71eac94fd48e46207b0316 > samza-rest/src/main/config/samza-rest.properties > 7be0b47d1466d2199ae278247e8d8152
Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job
> On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote: > > docs/learn/documentation/versioned/rest/resources/tasks.md, line 55 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552720#file1552720line55> > > > > What's the difference b/w containerId and containerName? > > Shanthoosh Venkataraman wrote: > Container Name is the unique name that identifies a container within a > job. Exposing this information is useful when killing containers(performing > related admin actions on it.). Currently container name is generated > prefixing container id with a string. > > Prateek Maheshwari wrote: > I don't think adding a separate container name concept is necessary. The > containerId is sufficient to uniquely identify the container. The > "samza-container" prefix makes sense in the MDC for disambiguation in the log > lines, but not sure what it buys us here. Removed container Name from the response. > On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote: > > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, > > lines 99-102 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552723#file1552723line99> > > > > Not required for the other place they're used? > > Shanthoosh Venkataraman wrote: > Yes. > > Prateek Maheshwari wrote: > "Yes, it's required" or "Yes, it's not required"? If the latter, why? Yes, it's required. It was added in the refactoring as well. > On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote: > > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala, > > line 104 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552722#file1552722line104> > > > > Don't think we need a Util method for a string concat. > > Shanthoosh Venkataraman wrote: > Container name is used in multiple places(SamzaContainer and > TasksResource). The way in which the container name is constructed from > container id could change in the future, hence this was added just to > encapsulate that here. > > Prateek Maheshwari wrote: > See comment above. Removed container name. Just exposing containerId. > On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote: > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java, > > line 52 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552732#file1552732line52> > > > > config.getConfigJobConfigFactory? Result of careless name refactoring. Fixed. > On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote: > > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, > > line 75 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552723#file1552723line75> > > > > Should take MetricsRegistry, not MetricsRegistryMap. Same everywhere > > else, here and in samza-rest. Making this change in samza-rest in straight forward. However, doing it in Samza-core is very hard. MetricsRegistryMap in this constructor is dependent upon by the constructor of the following classes ClusterBasedJobCoordinator, ContainerProcessManager, ContainerProcessManagerMetrics. Cost associated with this change would be touching a lot of classes and their associated tests. We should punt this for now and do it later. > On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote: > > samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala, line 30 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552721#file1552721line30> > > > > Prefer explicit imports Done. > On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote: > > docs/learn/documentation/versioned/rest/resources/tasks.md, line 122 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552720#file1552720line122> > > > > "within the installation path" Done. > On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote: > > docs/learn/documentation/versioned/rest/resources/tasks.md, line 116 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552720#file1552720line116> > > > > Don't split class names: "TaskProxyFactory", "TaskProxy". > > > > What's the for? Did you mean a ? > > > > "Has support to get" -> "Gets" > > > > Remove "abstraction" after SimpleInstallationRecord Done. - Shanthoosh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52168/#review154675 ---
Re: Review Request 53297: Initial version of adding metrics into samza rest.
> On Nov. 9, 2016, 12:31 a.m., Prateek Maheshwari wrote: > > samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java, > > line 36 > > <https://reviews.apache.org/r/53297/diff/2/?file=1556961#file1556961line36> > > > > Minor: private constructors for helper classes are pretty universal, > > don't think they need a comment. Done. > On Nov. 9, 2016, 12:31 a.m., Prateek Maheshwari wrote: > > docs/learn/documentation/versioned/rest/monitors.md, line 100 > > <https://reviews.apache.org/r/53297/diff/2/?file=1556960#file1556960line100> > > > > This section could be more concise. Changed, removed some of the docs. > On Nov. 9, 2016, 12:31 a.m., Prateek Maheshwari wrote: > > samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java, > > line 40 > > <https://reviews.apache.org/r/53297/diff/2/?file=1556961#file1556961line40> > > > > This javadoc doesn't add any more information than the method signature > > already provides. Prefer removing. Removed. > On Nov. 9, 2016, 12:31 a.m., Prateek Maheshwari wrote: > > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java, line 55 > > <https://reviews.apache.org/r/53297/diff/2/?file=1556964#file1556964line55> > > > > What is this config, and what does container refer to here? The > > samza-rest service container? Container name here refers to the execution unit from which the metrics gets reported. It's used when instantiating the MetricsReporter. If this configuration is undefined, it's defaulted to hostName on which SamzaRest is running. Resources will report metrics alongside monitors. Added relevent documentation for it. > On Nov. 9, 2016, 12:31 a.m., Prateek Maheshwari wrote: > > samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java, > > line 45 > > <https://reviews.apache.org/r/53297/diff/2/?file=1556961#file1556961line45> > > > > Not sure this specific method (for classloading this particular class > > from configs) deserves a new Util class. This is a general pattern used in > > other places, maybe we should extract that as a util instead. > > > > May be worth revisiting if/how we want to share code b/w samza-rest and > > samza-core. Ideally samza-rest shouldn't depend on samza-core, and shared > > stuff should be pulled out to samza-api or samza-common (which doesn't > > exist yet). Discussed offline. Extracting utils(and other common functionalities) shared between samza-core & samza-rest into samza-common is a longer term goal which is required to remove dependencies. However, for the scope of this patch, doing this will be a overkill. Punted for now. - Shanthoosh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53297/#review155380 --- On Nov. 9, 2016, 10:50 p.m., Shanthoosh Venkataraman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53297/ > --- > > (Updated Nov. 9, 2016, 10:50 p.m.) > > > Review request for samza and Jake Maes. > > > Repository: samza > > > Description > --- > > This patch aims at enabling users to define custom reporters to send metrics > from the monitors. Configurations required for the definition of the metrics > reporters follows the same convention as of the samza jobs. > > > Diffs > - > > docs/learn/documentation/versioned/rest/monitors.md > 46678bbe5fed99f767c3324dc9578ee1a64cec66 > docs/learn/documentation/versioned/rest/overview.md > c382f032843cce696a445ff110e87a8198cc96d7 > samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java > PRE-CREATION > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > e0468ee89c89fd720834461771ebb36475475bcb > > samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala > f24beb1e099dd44b15b475e0a4a7f70560c6965e > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java > 47b0663637f6db187d86961377ee3ee203b73fdb > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java > 2a3e83a24a5343bb53b93fc9d0a647c1b253714b > samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java > PRE-CREATION > > samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala > 8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca > > Diff: https://reviews.apache.org/r/53297/diff/ > > > Testing > --- > > Unit tests are done to verify the intended functionality. > > > Thanks, > > Shanthoosh Venkataraman > >
Re: Review Request 53297: Initial version of adding metrics into samza rest.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53297/ --- (Updated Nov. 9, 2016, 10:50 p.m.) Review request for samza and Jake Maes. Repository: samza Description --- This patch aims at enabling users to define custom reporters to send metrics from the monitors. Configurations required for the definition of the metrics reporters follows the same convention as of the samza jobs. Diffs (updated) - docs/learn/documentation/versioned/rest/monitors.md 46678bbe5fed99f767c3324dc9578ee1a64cec66 docs/learn/documentation/versioned/rest/overview.md c382f032843cce696a445ff110e87a8198cc96d7 samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java PRE-CREATION samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala e0468ee89c89fd720834461771ebb36475475bcb samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala f24beb1e099dd44b15b475e0a4a7f70560c6965e samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java 47b0663637f6db187d86961377ee3ee203b73fdb samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java 2a3e83a24a5343bb53b93fc9d0a647c1b253714b samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java PRE-CREATION samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala 8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca Diff: https://reviews.apache.org/r/53297/diff/ Testing --- Unit tests are done to verify the intended functionality. Thanks, Shanthoosh Venkataraman
Re: Review Request 53297: Initial version of adding metrics into samza rest.
> On Nov. 9, 2016, 12:09 a.m., Jagadish Venkatraman wrote: > > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java, line > > 65 > > <https://reviews.apache.org/r/53297/diff/2/?file=1556965#file1556965line65> > > > > Why is this taking in a concrete class - ReadableMetricsRegistry ? > > Prefer to have this `MetricsRegistry` instead. > > > > We should operate at the level of interfaces whereever possible. ReadableMetricsRegistry is an interface, not a concrete class. This particular type is required for registering with MetricsReporter instances. > On Nov. 9, 2016, 12:09 a.m., Jagadish Venkatraman wrote: > > docs/learn/documentation/versioned/rest/monitors.md, line 95 > > <https://reviews.apache.org/r/53297/diff/2/?file=1556960#file1556960line95> > > > > Documentation looks different (than the rest of the page). Would be > > nice if this was instructional in nature. > > > > /s/create/create and report. > > /s/theirs/yours Done. > On Nov. 9, 2016, 12:09 a.m., Jagadish Venkatraman wrote: > > docs/learn/documentation/versioned/rest/monitors.md, line 99 > > <https://reviews.apache.org/r/53297/diff/2/?file=1556960#file1556960line99> > > > > 1. s/Please refer/You can refer > > 2. `custom` is probably redundant. > > 3. The link and anchor text placement is weird. > > 4. Prefer to be precise with documentation. (For example, this line has > > the word `metrics reporters` thrice) Done. > On Nov. 9, 2016, 12:09 a.m., Jagadish Venkatraman wrote: > > docs/learn/documentation/versioned/rest/monitors.md, line 100 > > <https://reviews.apache.org/r/53297/diff/2/?file=1556960#file1556960line100> > > > > This line is not needed IMO. (If we choose to retain, it, should > > atleast be re-worded) Removed. > On Nov. 9, 2016, 12:09 a.m., Jagadish Venkatraman wrote: > > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java, line 55 > > <https://reviews.apache.org/r/53297/diff/2/?file=1556964#file1556964line55> > > > > How exactly is this used when metrics are reported? > > > > Is there a reason for this to be configurable / adding a new config > > key? > > > > If it's configurable, we should document it somewhere? Container name here refers to the execution unit from which the metrics gets reported. It's used when instantiating the MetricsReporter. If this configuration is undefined, it's defaulted to hostName on which SamzaRest is running. Resources will report metrics alongside monitors. Added relevent documentation for it. - Shanthoosh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53297/#review155372 --- On Nov. 9, 2016, 10:50 p.m., Shanthoosh Venkataraman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53297/ > --- > > (Updated Nov. 9, 2016, 10:50 p.m.) > > > Review request for samza and Jake Maes. > > > Repository: samza > > > Description > --- > > This patch aims at enabling users to define custom reporters to send metrics > from the monitors. Configurations required for the definition of the metrics > reporters follows the same convention as of the samza jobs. > > > Diffs > - > > docs/learn/documentation/versioned/rest/monitors.md > 46678bbe5fed99f767c3324dc9578ee1a64cec66 > docs/learn/documentation/versioned/rest/overview.md > c382f032843cce696a445ff110e87a8198cc96d7 > samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java > PRE-CREATION > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > e0468ee89c89fd720834461771ebb36475475bcb > > samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala > f24beb1e099dd44b15b475e0a4a7f70560c6965e > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java > 47b0663637f6db187d86961377ee3ee203b73fdb > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java > 2a3e83a24a5343bb53b93fc9d0a647c1b253714b > samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java > PRE-CREATION > > samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala > 8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca > > Diff: https://reviews.apache.org/r/53297/diff/ > > > Testing > --- > > Unit tests are done to verify the intended functionality. > > > Thanks, > > Shanthoosh Venkataraman > >
Re: Review Request 53297: Initial version of adding metrics into samza rest.
> On Nov. 9, 2016, 12:09 a.m., Jagadish Venkatraman wrote: > > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java, line 55 > > <https://reviews.apache.org/r/53297/diff/2/?file=1556964#file1556964line55> > > > > How exactly is this used when metrics are reported? > > > > Is there a reason for this to be configurable / adding a new config > > key? > > > > If it's configurable, we should document it somewhere? > > Shanthoosh Venkataraman wrote: > Container name here refers to the execution unit from which the metrics > gets reported. It's used when instantiating the MetricsReporter. If this > configuration is undefined, it's defaulted to hostName on which SamzaRest is > running. Resources will report metrics alongside monitors. Added relevent > documentation for it. > > Jagadish Venkatraman wrote: > Is there a use case where I would want to configure a different container > name? If not, this need not be a config? (we can default it to `samza-rest`) For LI use case, using hostname(or SamzaRest) would suffice. My understanding is that the containerName denotes either the logical host or the physical host or some unique tag to identify the origin from which the metrics gets reported from. This container name could be appended into metrics reported depending upon MetricsReporter implementations. Hence, I think it will be useful to let users configure it. For instance, some implementations could generate a unique string as container name based upon environment and populate it in the config to uniquely identify the environment it's reporting the metrics from. - Shanthoosh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53297/#review155372 --- On Nov. 9, 2016, 10:50 p.m., Shanthoosh Venkataraman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53297/ > --- > > (Updated Nov. 9, 2016, 10:50 p.m.) > > > Review request for samza and Jake Maes. > > > Repository: samza > > > Description > --- > > This patch aims at enabling users to define custom reporters to send metrics > from the monitors. Configurations required for the definition of the metrics > reporters follows the same convention as of the samza jobs. > > > Diffs > - > > docs/learn/documentation/versioned/rest/monitors.md > 46678bbe5fed99f767c3324dc9578ee1a64cec66 > docs/learn/documentation/versioned/rest/overview.md > c382f032843cce696a445ff110e87a8198cc96d7 > samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java > PRE-CREATION > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > e0468ee89c89fd720834461771ebb36475475bcb > > samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala > f24beb1e099dd44b15b475e0a4a7f70560c6965e > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java > 47b0663637f6db187d86961377ee3ee203b73fdb > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java > 2a3e83a24a5343bb53b93fc9d0a647c1b253714b > samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java > PRE-CREATION > > samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala > 8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca > > Diff: https://reviews.apache.org/r/53297/diff/ > > > Testing > --- > > Unit tests are done to verify the intended functionality. > > > Thanks, > > Shanthoosh Venkataraman > >
Re: Review Request 53297: Initial version of adding metrics into samza rest.
> On Nov. 9, 2016, 11:18 p.m., Prateek Maheshwari wrote: > > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java, line 55 > > <https://reviews.apache.org/r/53297/diff/2/?file=1556964#file1556964line55> > > > > What's the execution unit here? The samza-rest server? The monitor? The > > container being monitored? I think `execution unit` is a misnomer. By execution unit, I meant the origin of the metrics (physical hostname in which the samza rest process is running/SamzaRestMonitor literal/SamzaRestResource literal etc) - Shanthoosh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53297/#review155511 ------- On Nov. 9, 2016, 10:50 p.m., Shanthoosh Venkataraman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53297/ > --- > > (Updated Nov. 9, 2016, 10:50 p.m.) > > > Review request for samza and Jake Maes. > > > Repository: samza > > > Description > --- > > This patch aims at enabling users to define custom reporters to send metrics > from the monitors. Configurations required for the definition of the metrics > reporters follows the same convention as of the samza jobs. > > > Diffs > - > > docs/learn/documentation/versioned/rest/monitors.md > 46678bbe5fed99f767c3324dc9578ee1a64cec66 > docs/learn/documentation/versioned/rest/overview.md > c382f032843cce696a445ff110e87a8198cc96d7 > samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java > PRE-CREATION > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > e0468ee89c89fd720834461771ebb36475475bcb > > samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala > f24beb1e099dd44b15b475e0a4a7f70560c6965e > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java > 47b0663637f6db187d86961377ee3ee203b73fdb > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java > 2a3e83a24a5343bb53b93fc9d0a647c1b253714b > samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java > PRE-CREATION > > samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala > 8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca > > Diff: https://reviews.apache.org/r/53297/diff/ > > > Testing > --- > > Unit tests are done to verify the intended functionality. > > > Thanks, > > Shanthoosh Venkataraman > >
Re: Review Request 53297: Initial version of adding metrics into samza rest.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53297/ --- (Updated Nov. 10, 2016, 12:04 a.m.) Review request for samza and Jake Maes. Repository: samza Description --- This patch aims at enabling users to define custom reporters to send metrics from the monitors. Configurations required for the definition of the metrics reporters follows the same convention as of the samza jobs. Diffs (updated) - docs/learn/documentation/versioned/rest/monitors.md 46678bbe5fed99f767c3324dc9578ee1a64cec66 docs/learn/documentation/versioned/rest/overview.md c382f032843cce696a445ff110e87a8198cc96d7 samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java PRE-CREATION samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala e0468ee89c89fd720834461771ebb36475475bcb samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala f24beb1e099dd44b15b475e0a4a7f70560c6965e samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java 47b0663637f6db187d86961377ee3ee203b73fdb samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java 2a3e83a24a5343bb53b93fc9d0a647c1b253714b samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java PRE-CREATION samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala 8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca Diff: https://reviews.apache.org/r/53297/diff/ Testing --- Unit tests are done to verify the intended functionality. Thanks, Shanthoosh Venkataraman
Re: Review Request 53297: Initial version of adding metrics into samza rest.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53297/ --- (Updated Nov. 10, 2016, 1:04 a.m.) Review request for samza and Jake Maes. Repository: samza Description --- This patch aims at enabling users to define custom reporters to send metrics from the monitors. Configurations required for the definition of the metrics reporters follows the same convention as of the samza jobs. Diffs (updated) - docs/learn/documentation/versioned/rest/monitors.md 46678bbe5fed99f767c3324dc9578ee1a64cec66 samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java PRE-CREATION samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala e0468ee89c89fd720834461771ebb36475475bcb samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala f24beb1e099dd44b15b475e0a4a7f70560c6965e samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java 47b0663637f6db187d86961377ee3ee203b73fdb samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java 2a3e83a24a5343bb53b93fc9d0a647c1b253714b samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java PRE-CREATION samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala 8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca Diff: https://reviews.apache.org/r/53297/diff/ Testing --- Unit tests are done to verify the intended functionality. Thanks, Shanthoosh Venkataraman
Re: Review Request 53297: Initial version of adding metrics into samza rest.
> On Nov. 9, 2016, 12:09 a.m., Jagadish Venkatraman wrote: > > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java, line 55 > > <https://reviews.apache.org/r/53297/diff/2/?file=1556964#file1556964line55> > > > > How exactly is this used when metrics are reported? > > > > Is there a reason for this to be configurable / adding a new config > > key? > > > > If it's configurable, we should document it somewhere? > > Shanthoosh Venkataraman wrote: > Container name here refers to the execution unit from which the metrics > gets reported. It's used when instantiating the MetricsReporter. If this > configuration is undefined, it's defaulted to hostName on which SamzaRest is > running. Resources will report metrics alongside monitors. Added relevent > documentation for it. > > Jagadish Venkatraman wrote: > Is there a use case where I would want to configure a different container > name? If not, this need not be a config? (we can default it to `samza-rest`) > > Shanthoosh Venkataraman wrote: > For LI use case, using hostname(or SamzaRest) would suffice. > My understanding is that the containerName denotes either the logical > host or the physical host or some unique tag to identify the origin from > which the metrics gets reported from. > This container name could be appended into metrics reported depending > upon MetricsReporter implementations. > Hence, I think it will be useful to let users configure it. > For instance, some implementations could generate a unique string as > container name based upon environment and populate it in the config to > uniquely identify the environment it's reporting the metrics from. Discussed offline. Exposing containerName through samza-rest config might be unnecessary as of now. Hardcoding it to hostname on which `samza-rest` is running. - Shanthoosh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53297/#review155372 --- On Nov. 10, 2016, 1:04 a.m., Shanthoosh Venkataraman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53297/ > --- > > (Updated Nov. 10, 2016, 1:04 a.m.) > > > Review request for samza and Jake Maes. > > > Repository: samza > > > Description > --- > > This patch aims at enabling users to define custom reporters to send metrics > from the monitors. Configurations required for the definition of the metrics > reporters follows the same convention as of the samza jobs. > > > Diffs > - > > docs/learn/documentation/versioned/rest/monitors.md > 46678bbe5fed99f767c3324dc9578ee1a64cec66 > samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java > PRE-CREATION > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > e0468ee89c89fd720834461771ebb36475475bcb > > samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala > f24beb1e099dd44b15b475e0a4a7f70560c6965e > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java > 47b0663637f6db187d86961377ee3ee203b73fdb > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java > 2a3e83a24a5343bb53b93fc9d0a647c1b253714b > samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java > PRE-CREATION > > samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala > 8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca > > Diff: https://reviews.apache.org/r/53297/diff/ > > > Testing > --- > > Unit tests are done to verify the intended functionality. > > > Thanks, > > Shanthoosh Venkataraman > >
Re: Review Request 53297: Initial version of adding metrics into samza rest.
> On Nov. 9, 2016, 11:18 p.m., Prateek Maheshwari wrote: > > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java, line 55 > > <https://reviews.apache.org/r/53297/diff/2/?file=1556964#file1556964line55> > > > > What's the execution unit here? The samza-rest server? The monitor? The > > container being monitored? > > Shanthoosh Venkataraman wrote: > I think `execution unit` is a misnomer. By execution unit, I meant the > origin of the metrics (physical hostname in which the samza rest process is > running/SamzaRestMonitor literal/SamzaRestResource literal etc) Discussed offline. Exposing containerName through samza-rest config might be unnecessary as of now. Hardcoding it to hostname on which `samza-rest` is running. - Shanthoosh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53297/#review155511 --- On Nov. 10, 2016, 1:04 a.m., Shanthoosh Venkataraman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53297/ > --- > > (Updated Nov. 10, 2016, 1:04 a.m.) > > > Review request for samza and Jake Maes. > > > Repository: samza > > > Description > --- > > This patch aims at enabling users to define custom reporters to send metrics > from the monitors. Configurations required for the definition of the metrics > reporters follows the same convention as of the samza jobs. > > > Diffs > - > > docs/learn/documentation/versioned/rest/monitors.md > 46678bbe5fed99f767c3324dc9578ee1a64cec66 > samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java > PRE-CREATION > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > e0468ee89c89fd720834461771ebb36475475bcb > > samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala > f24beb1e099dd44b15b475e0a4a7f70560c6965e > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java > 47b0663637f6db187d86961377ee3ee203b73fdb > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java > 2a3e83a24a5343bb53b93fc9d0a647c1b253714b > samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java > PRE-CREATION > > samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala > 8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca > > Diff: https://reviews.apache.org/r/53297/diff/ > > > Testing > --- > > Unit tests are done to verify the intended functionality. > > > Thanks, > > Shanthoosh Venkataraman > >
Re: Review Request 53297: Initial version of adding metrics into samza rest.
> On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote: > > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java, line 27 > > <https://reviews.apache.org/r/53297/diff/5/?file=1560179#file1560179line27> > > > > Unused import. Removed. > On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote: > > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java, line > > 164 > > <https://reviews.apache.org/r/53297/diff/5/?file=1560180#file1560180line164> > > > > debug? I think this should be info. It will be useful to know the registered reporters while debugging. > On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote: > > samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java, > > line 39 > > <https://reviews.apache.org/r/53297/diff/5/?file=1560176#file1560176line39> > > > > I meant remove the whole javadoc including @params. There's no useful > > additional information here. Removed. > On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote: > > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java, line > > 107 > > <https://reviews.apache.org/r/53297/diff/5/?file=1560180#file1560180line107> > > > > Should this be called during start()? Same for stop(). To accomplish this, both(SamzaRestService,SamzaMonitorService) of them had to be coupled. Logically they are seperate entities. One responsible for resources and other monitors. Currently MonitorService & RestService are not coupled with each other. It would be useful keep them seperate(In the future, we could have SamzaRestService and SamzaMonitorService deployed seperately when we have too many(Monitors & Resources) in them). I would prefer not to have this coupling. > On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote: > > samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala, > > line 56 > > <https://reviews.apache.org/r/53297/diff/5/?file=1560182#file1560182line56> > > > > Maybe use JavaCoverters._ and .asScala? Done. > On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote: > > samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala, > > line 46 > > <https://reviews.apache.org/r/53297/diff/5/?file=1560178#file1560178line46> > > > > Maybe use JavaCoverters._ and .asScala? Done. > On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote: > > samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java, > > line 30 > > <https://reviews.apache.org/r/53297/diff/5/?file=1560176#file1560176line30> > > > > Remove "contains reusable methods which" and "based upon > > configuration". Don't mind removing the whole comment either. Removed. > On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote: > > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java, line > > 103 > > <https://reviews.apache.org/r/53297/diff/5/?file=1560180#file1560180line103> > > > > Don't understand this interface. What's a SchedulingProvider, and how > > is it different than a ScheduledExecutorService? > > > > schedulingProvider should be probably be stopped in stop() in this > > class and not in SamzaMonitorService since this class owns it. Either that, > > or move instantiation to SamzaMonitorService too. SchedulingProvider interface looks similar to that of ExecutorService. Not sure of the value add this interface provides. I guess, this is not required and we could just use ExeuctorService. Added a jira for it, since it's not in the scope of this patch. We could re-visit it later. > On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote: > > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java, line > > 166 > > <https://reviews.apache.org/r/53297/diff/5/?file=1560180#file1560180line166> > > > > debug? I think this should be info. Will be useful to know this when debugging. > On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote: > > docs/learn/documentation/versioned/rest/monitors.md, line 98 > > <https://reviews.apache.org/r/53297/diff/5/?file=1560175#file1560175line98> > > > > Maybe: "are the same as that of Samza jobs." and make "that of Samza > > jobs" the configuration link and remove next line? Done. > On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote: > > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java, line > > 91 > > <https://reviews.apache.org/r/53297/diff/5/?file=1560180#file1560180line
Re: Review Request 53297: Initial version of adding metrics into samza rest.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53297/ --- (Updated Nov. 11, 2016, 12:22 a.m.) Review request for samza and Jake Maes. Repository: samza Description --- This patch aims at enabling users to define custom reporters to send metrics from the monitors. Configurations required for the definition of the metrics reporters follows the same convention as of the samza jobs. Diffs (updated) - docs/learn/documentation/versioned/rest/monitors.md 46678bbe5fed99f767c3324dc9578ee1a64cec66 samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java PRE-CREATION samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala e0468ee89c89fd720834461771ebb36475475bcb samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala f24beb1e099dd44b15b475e0a4a7f70560c6965e samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java 2a3e83a24a5343bb53b93fc9d0a647c1b253714b samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java PRE-CREATION samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala 8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca Diff: https://reviews.apache.org/r/53297/diff/ Testing --- Unit tests are done to verify the intended functionality. Thanks, Shanthoosh Venkataraman
Re: Review Request 53297: Initial version of adding metrics into samza rest.
> On Nov. 5, 2016, 6:47 p.m., Jake Maes wrote: > > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java, line > > 75 > > <https://reviews.apache.org/r/53297/diff/1/?file=1549407#file1549407line75> > > > > I don't think the MetricsConfig constructure takes a subset. > > > > I think it takes the root and expects to find the "metrics" prefix > > Jake Maes wrote: > s/constructure/constructor > > phonetic brain fail > > Shanthoosh Venkataraman wrote: > Yes, that's true. It expects the root and finds the metrics prefix. > Hence, stripPrefix is passed on as false, so that prefix isn't removed. This > just selects the config subset that starts with METRICS_PREFIX, without > removing the prefix. The goal is to not pass on the entire config object and > pass only metrics related config into MetricsConfig constructor. > > Prateek Maheshwari wrote: > Regarding the goal: That's not how the XConfig classes are intended to be > used, so it's not really necessary to do this. For example, see the implicit > conversion b/w Config and XConfig. Prefer passing in config. Done. - Shanthoosh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53297/#review155061 --- On Nov. 10, 2016, 1:04 a.m., Shanthoosh Venkataraman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53297/ > --- > > (Updated Nov. 10, 2016, 1:04 a.m.) > > > Review request for samza and Jake Maes. > > > Repository: samza > > > Description > --- > > This patch aims at enabling users to define custom reporters to send metrics > from the monitors. Configurations required for the definition of the metrics > reporters follows the same convention as of the samza jobs. > > > Diffs > - > > docs/learn/documentation/versioned/rest/monitors.md > 46678bbe5fed99f767c3324dc9578ee1a64cec66 > samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java > PRE-CREATION > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > e0468ee89c89fd720834461771ebb36475475bcb > > samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala > f24beb1e099dd44b15b475e0a4a7f70560c6965e > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java > 47b0663637f6db187d86961377ee3ee203b73fdb > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java > 2a3e83a24a5343bb53b93fc9d0a647c1b253714b > samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java > PRE-CREATION > > samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala > 8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca > > Diff: https://reviews.apache.org/r/53297/diff/ > > > Testing > --- > > Unit tests are done to verify the intended functionality. > > > Thanks, > > Shanthoosh Venkataraman > >
Re: Review Request 53297: Initial version of adding metrics into samza rest.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53297/ --- (Updated Nov. 16, 2016, 5:44 a.m.) Review request for samza and Jake Maes. Repository: samza Description (updated) --- This patch aims at enabling users to define custom reporters to send metrics from the monitors. Configurations required for the definition of the metrics reporters follows the same convention as of the samza jobs. Updated diff = Adding monitorName in MonitorFactory getMonitor method to enable creation of namespaced metrics from each of the monitor. monitorName can be used by MonitorFactory implementations in the metrics that are created and reported from them. Diffs (updated) - docs/learn/documentation/versioned/rest/monitors.md 46678bbe5fed99f767c3324dc9578ee1a64cec66 samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java PRE-CREATION samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala e0468ee89c89fd720834461771ebb36475475bcb samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala f24beb1e099dd44b15b475e0a4a7f70560c6965e samza-rest/src/main/java/org/apache/samza/monitor/MonitorFactory.java c38a5d8f737ee5cf39a4876697025fa9cd300bff samza-rest/src/main/java/org/apache/samza/monitor/MonitorLoader.java dcf9e5734b1d97e37e4fd0e89cc7f525a37a0d65 samza-rest/src/main/java/org/apache/samza/monitor/SamzaMonitorService.java 754ad82af9ef0f2334ec629160be39d183df1b38 samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java 2a3e83a24a5343bb53b93fc9d0a647c1b253714b samza-rest/src/test/java/org/apache/samza/monitor/TestMonitorService.java e75f4946f38523284921798578f2a3db9c02f599 samza-rest/src/test/java/org/apache/samza/monitor/mock/DummyMonitorFactory.java ccc534efc8594837f83b9126ca3d4ee22d8fbb05 samza-rest/src/test/java/org/apache/samza/monitor/mock/ExceptionThrowingMonitorFactory.java af414b46dc5c2ff1579deb2c247cd94388a0b3ea samza-rest/src/test/java/org/apache/samza/monitor/mock/MockMonitorFactory.java 5b19001eb5db51b6cc4440cf74be92f285ab0209 samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java PRE-CREATION samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala 8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca Diff: https://reviews.apache.org/r/53297/diff/ Testing --- Unit tests are done to verify the intended functionality. Thanks, Shanthoosh Venkataraman
Re: Review Request 53297: Initial version of adding metrics into samza rest.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53297/ --- (Updated Nov. 18, 2016, 10:18 p.m.) Review request for samza and Jake Maes. Repository: samza Description (updated) --- This patch aims at enabling users to define custom reporters to send metrics from the monitors. Configurations required for the definition of the metrics reporters follows the same convention as of the samza jobs. Updated diff = Adding monitorName in MonitorFactory getMonitor method to enable creation of namespaced metrics from each of the monitor. monitorName can be used by MonitorFactory implementations in the metrics that are created and reported from them. Jira associated with the patch : https://issues.apache.org/jira/browse/SAMZA-1049 Diffs - docs/learn/documentation/versioned/rest/monitors.md 46678bbe5fed99f767c3324dc9578ee1a64cec66 samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java PRE-CREATION samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala e0468ee89c89fd720834461771ebb36475475bcb samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala f24beb1e099dd44b15b475e0a4a7f70560c6965e samza-rest/src/main/java/org/apache/samza/monitor/MonitorFactory.java c38a5d8f737ee5cf39a4876697025fa9cd300bff samza-rest/src/main/java/org/apache/samza/monitor/MonitorLoader.java dcf9e5734b1d97e37e4fd0e89cc7f525a37a0d65 samza-rest/src/main/java/org/apache/samza/monitor/SamzaMonitorService.java 754ad82af9ef0f2334ec629160be39d183df1b38 samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java 2a3e83a24a5343bb53b93fc9d0a647c1b253714b samza-rest/src/test/java/org/apache/samza/monitor/TestMonitorService.java e75f4946f38523284921798578f2a3db9c02f599 samza-rest/src/test/java/org/apache/samza/monitor/mock/DummyMonitorFactory.java ccc534efc8594837f83b9126ca3d4ee22d8fbb05 samza-rest/src/test/java/org/apache/samza/monitor/mock/ExceptionThrowingMonitorFactory.java af414b46dc5c2ff1579deb2c247cd94388a0b3ea samza-rest/src/test/java/org/apache/samza/monitor/mock/MockMonitorFactory.java 5b19001eb5db51b6cc4440cf74be92f285ab0209 samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java PRE-CREATION samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala 8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca Diff: https://reviews.apache.org/r/53297/diff/ Testing --- Unit tests are done to verify the intended functionality. Thanks, Shanthoosh Venkataraman
Re: Review Request 52476: Do not load task store which are older than delete tombstones.
> On Jan. 25, 2017, 10:28 p.m., Prateek Maheshwari wrote: > > samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala, > > line 97 > > <https://reviews.apache.org/r/52476/diff/5/?file=1543710#file1543710line97> > > > > "Got default storage ..." Changed. > On Jan. 25, 2017, 10:28 p.m., Prateek Maheshwari wrote: > > samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala, > > line 137 > > <https://reviews.apache.org/r/52476/diff/5/?file=1543710#file1543710line137> > > > > No space before ":" here and elsewhere, including method type > > annotations (e.g. line 169, 185). Fixed. > On Jan. 25, 2017, 10:28 p.m., Prateek Maheshwari wrote: > > samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala, > > line 152 > > <https://reviews.apache.org/r/52476/diff/5/?file=1543710#file1543710line152> > > > > We should log which directory we're using for the store here at INFO. Added a log statement. - Shanthoosh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52476/#review163035 --- On Oct. 22, 2016, 10:06 p.m., Shanthoosh Venkataraman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52476/ > --- > > (Updated Oct. 22, 2016, 10:06 p.m.) > > > Review request for samza. > > > Repository: samza > > > Description > --- > > Every local task store is backed up by a kafka changelog topic. Due to log > compaction, delete tombstones of the changelog topic have a ttl of > delete.retention.ms. Replaying the events from the changelog that has missing > delete tombstones, would result in creation of an inconsistent local > store(due to the missing of some delete events). This patch deletes the local > stores in which difference between current time and last modified time of the > offset file is greater than delete.retention.ms during the container startup. > > > Diffs > - > > samza-core/src/main/java/org/apache/samza/storage/StorageRecovery.java > 9329edf7d724f3a0d9235354bb77936f713e3b5f > samza-core/src/main/scala/org/apache/samza/config/StorageConfig.scala > be3f1068f7921c9c8f8697577efea6580672813e > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > 05a996c98075ea8ed3767af666b9beeb1933f2a6 > samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala > 0b7bcdda1639eea8239a69c31bdf42558e9077d2 > > samza-core/src/test/scala/org/apache/samza/storage/TestTaskStorageManager.scala > 4d40f520e54beb643acd8410c772b75e2f6a9162 > samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala > 973ab8cfb3d248bec7efe5e338f5e667f097556d > > Diff: https://reviews.apache.org/r/52476/diff/ > > > Testing > --- > > Unit testing and manual testing has been done to verify the functionality. > > > Thanks, > > Shanthoosh Venkataraman > >
Re: Review Request 52476: Do not load task store which are older than delete tombstones.
> On Oct. 24, 2016, 9:43 p.m., Prateek Maheshwari wrote: > > samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala, > > line 138 > > <https://reviews.apache.org/r/52476/diff/5/?file=1543710#file1543710line138> > > > > s/is greater than/is older than. Fixed. > On Oct. 24, 2016, 9:43 p.m., Prateek Maheshwari wrote: > > samza-core/src/main/scala/org/apache/samza/config/StorageConfig.scala, line > > 32 > > <https://reviews.apache.org/r/52476/diff/5/?file=1543708#file1543708line32> > > > > s/CHANGE_LOG/CHANGELOG Fixed. - Shanthoosh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52476/#review153741 ------- On Oct. 22, 2016, 10:06 p.m., Shanthoosh Venkataraman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52476/ > --- > > (Updated Oct. 22, 2016, 10:06 p.m.) > > > Review request for samza. > > > Repository: samza > > > Description > --- > > Every local task store is backed up by a kafka changelog topic. Due to log > compaction, delete tombstones of the changelog topic have a ttl of > delete.retention.ms. Replaying the events from the changelog that has missing > delete tombstones, would result in creation of an inconsistent local > store(due to the missing of some delete events). This patch deletes the local > stores in which difference between current time and last modified time of the > offset file is greater than delete.retention.ms during the container startup. > > > Diffs > - > > samza-core/src/main/java/org/apache/samza/storage/StorageRecovery.java > 9329edf7d724f3a0d9235354bb77936f713e3b5f > samza-core/src/main/scala/org/apache/samza/config/StorageConfig.scala > be3f1068f7921c9c8f8697577efea6580672813e > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > 05a996c98075ea8ed3767af666b9beeb1933f2a6 > samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala > 0b7bcdda1639eea8239a69c31bdf42558e9077d2 > > samza-core/src/test/scala/org/apache/samza/storage/TestTaskStorageManager.scala > 4d40f520e54beb643acd8410c772b75e2f6a9162 > samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala > 973ab8cfb3d248bec7efe5e338f5e667f097556d > > Diff: https://reviews.apache.org/r/52476/diff/ > > > Testing > --- > > Unit testing and manual testing has been done to verify the functionality. > > > Thanks, > > Shanthoosh Venkataraman > >
Re: Review Request 52476: SAMZA-1083 : Do not load task store which are older than delete tombstones.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52476/ --- (Updated Feb. 8, 2017, 7:09 p.m.) Review request for samza. Summary (updated) - SAMZA-1083 : Do not load task store which are older than delete tombstones. Repository: samza Description --- Every local task store is backed up by a kafka changelog topic. Due to log compaction, delete tombstones of the changelog topic have a ttl of delete.retention.ms. Replaying the events from the changelog that has missing delete tombstones, would result in creation of an inconsistent local store(due to the missing of some delete events). This patch deletes the local stores in which difference between current time and last modified time of the offset file is greater than delete.retention.ms during the container startup. Diffs - samza-core/src/main/java/org/apache/samza/storage/StorageRecovery.java 9329edf7d724f3a0d9235354bb77936f713e3b5f samza-core/src/main/scala/org/apache/samza/config/StorageConfig.scala be3f1068f7921c9c8f8697577efea6580672813e samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6 samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala 0b7bcdda1639eea8239a69c31bdf42558e9077d2 samza-core/src/test/scala/org/apache/samza/storage/TestTaskStorageManager.scala 4d40f520e54beb643acd8410c772b75e2f6a9162 samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala 973ab8cfb3d248bec7efe5e338f5e667f097556d Diff: https://reviews.apache.org/r/52476/diff/ Testing --- Unit testing and manual testing has been done to verify the functionality. Thanks, Shanthoosh Venkataraman
Re: Review Request 52476: SAMZA-1083 : Do not load task store which are older than delete tombstones.
> On Feb. 8, 2017, 5:17 p.m., Jake Maes wrote: > > samza-core/src/main/scala/org/apache/samza/config/StorageConfig.scala, line > > 33 > > <https://reviews.apache.org/r/52476/diff/5/?file=1543708#file1543708line33> > > > > nit: TimeUnit.DAYS.toMillis(1) Fixed. - Shanthoosh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52476/#review164701 --- On Feb. 8, 2017, 7:09 p.m., Shanthoosh Venkataraman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52476/ > --- > > (Updated Feb. 8, 2017, 7:09 p.m.) > > > Review request for samza. > > > Repository: samza > > > Description > --- > > Every local task store is backed up by a kafka changelog topic. Due to log > compaction, delete tombstones of the changelog topic have a ttl of > delete.retention.ms. Replaying the events from the changelog that has missing > delete tombstones, would result in creation of an inconsistent local > store(due to the missing of some delete events). This patch deletes the local > stores in which difference between current time and last modified time of the > offset file is greater than delete.retention.ms during the container startup. > > > Diffs > - > > samza-core/src/main/java/org/apache/samza/storage/StorageRecovery.java > 9329edf7d724f3a0d9235354bb77936f713e3b5f > samza-core/src/main/scala/org/apache/samza/config/StorageConfig.scala > be3f1068f7921c9c8f8697577efea6580672813e > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > 05a996c98075ea8ed3767af666b9beeb1933f2a6 > samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala > 0b7bcdda1639eea8239a69c31bdf42558e9077d2 > > samza-core/src/test/scala/org/apache/samza/storage/TestTaskStorageManager.scala > 4d40f520e54beb643acd8410c772b75e2f6a9162 > samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala > 973ab8cfb3d248bec7efe5e338f5e667f097556d > > Diff: https://reviews.apache.org/r/52476/diff/ > > > Testing > --- > > Unit testing and manual testing has been done to verify the functionality. > > > Thanks, > > Shanthoosh Venkataraman > >
Re: Review Request 52476: SAMZA-1083 : Do not load task store which are older than delete tombstones.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52476/ --- (Updated Feb. 8, 2017, 8 p.m.) Review request for samza. Repository: samza Description --- Every local task store is backed up by a kafka changelog topic. Due to log compaction, delete tombstones of the changelog topic have a ttl of delete.retention.ms. Replaying the events from the changelog that has missing delete tombstones, would result in creation of an inconsistent local store(due to the missing of some delete events). This patch deletes the local stores in which difference between current time and last modified time of the offset file is greater than delete.retention.ms during the container startup. Diffs (updated) - samza-core/src/main/java/org/apache/samza/storage/StorageRecovery.java 9329edf7d724f3a0d9235354bb77936f713e3b5f samza-core/src/main/scala/org/apache/samza/config/StorageConfig.scala a3587d0a40c57374ee1742234929d444e381e42d samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala c3308bfd7de04c335fef6cb66baa29286a230080 samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala 0b7bcdda1639eea8239a69c31bdf42558e9077d2 samza-core/src/test/scala/org/apache/samza/storage/TestTaskStorageManager.scala 4d40f520e54beb643acd8410c772b75e2f6a9162 samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala 9320cf744ff90d647a198b51cb06d2a526fe68fa Diff: https://reviews.apache.org/r/52476/diff/ Testing --- Unit testing and manual testing has been done to verify the functionality. Thanks, Shanthoosh Venkataraman
Re: Review Request 52476: SAMZA-1083 : Do not load task store which are older than delete tombstones.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52476/ --- (Updated Feb. 8, 2017, 9:37 p.m.) Review request for samza. Repository: samza Description --- Every local task store is backed up by a kafka changelog topic. Due to log compaction, delete tombstones of the changelog topic have a ttl of delete.retention.ms. Replaying the events from the changelog that has missing delete tombstones, would result in creation of an inconsistent local store(due to the missing of some delete events). This patch deletes the local stores in which difference between current time and last modified time of the offset file is greater than delete.retention.ms during the container startup. Diffs (updated) - samza-core/src/main/java/org/apache/samza/storage/StorageRecovery.java 9329edf7d724f3a0d9235354bb77936f713e3b5f samza-core/src/main/scala/org/apache/samza/config/StorageConfig.scala a3587d0a40c57374ee1742234929d444e381e42d samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala c3308bfd7de04c335fef6cb66baa29286a230080 samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala 0b7bcdda1639eea8239a69c31bdf42558e9077d2 samza-core/src/test/scala/org/apache/samza/storage/TestTaskStorageManager.scala 4d40f520e54beb643acd8410c772b75e2f6a9162 samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala 9320cf744ff90d647a198b51cb06d2a526fe68fa Diff: https://reviews.apache.org/r/52476/diff/ Testing --- Unit testing and manual testing has been done to verify the functionality. Thanks, Shanthoosh Venkataraman
Re: Review Request 52476: SAMZA-1083 : Do not load task store which are older than delete tombstones.
> On Feb. 8, 2017, 8:43 p.m., Jake Maes wrote: > > samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala, > > line 177 > > <https://reviews.apache.org/r/52476/diff/4-6/?file=1541857#file1541857line177> > > > > Looks like this log statement belongs in an else-block. It says the > > store is stale, but in this block, we've determined hasValidOffsetFile=true. > > > > Also the wording isn't entirely accurate. The offset file may be > > present, but empty. It might be better to say "Offset file is not valid for > > store: %s" My bad, this minor thing was missed while refactoring. Thanks for pointing it out. Fixed. - Shanthoosh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52476/#review164765 ------- On Feb. 8, 2017, 9:37 p.m., Shanthoosh Venkataraman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52476/ > --- > > (Updated Feb. 8, 2017, 9:37 p.m.) > > > Review request for samza. > > > Repository: samza > > > Description > --- > > Every local task store is backed up by a kafka changelog topic. Due to log > compaction, delete tombstones of the changelog topic have a ttl of > delete.retention.ms. Replaying the events from the changelog that has missing > delete tombstones, would result in creation of an inconsistent local > store(due to the missing of some delete events). This patch deletes the local > stores in which difference between current time and last modified time of the > offset file is greater than delete.retention.ms during the container startup. > > > Diffs > - > > samza-core/src/main/java/org/apache/samza/storage/StorageRecovery.java > 9329edf7d724f3a0d9235354bb77936f713e3b5f > samza-core/src/main/scala/org/apache/samza/config/StorageConfig.scala > a3587d0a40c57374ee1742234929d444e381e42d > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > c3308bfd7de04c335fef6cb66baa29286a230080 > samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala > 0b7bcdda1639eea8239a69c31bdf42558e9077d2 > > samza-core/src/test/scala/org/apache/samza/storage/TestTaskStorageManager.scala > 4d40f520e54beb643acd8410c772b75e2f6a9162 > samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala > 9320cf744ff90d647a198b51cb06d2a526fe68fa > > Diff: https://reviews.apache.org/r/52476/diff/ > > > Testing > --- > > Unit testing and manual testing has been done to verify the functionality. > > > Thanks, > > Shanthoosh Venkataraman > >
Re: Review Request 52476: SAMZA-1083 : Do not load task store which are older than delete tombstones.
> On Feb. 8, 2017, 10:06 p.m., Prateek Maheshwari wrote:
> > samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala,
> > line 196
> > <https://reviews.apache.org/r/52476/diff/7/?file=1627958#file1627958line196>
> >
> > s/in the store/for the store.
Fixed.
> On Feb. 8, 2017, 10:06 p.m., Prateek Maheshwari wrote:
> > samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala,
> > line 199
> > <https://reviews.apache.org/r/52476/diff/7/?file=1627958#file1627958line199>
> >
> > No space before colon, here and other places in this file.
Fixed at all the places.
> On Feb. 8, 2017, 10:06 p.m., Prateek Maheshwari wrote:
> > samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala,
> > line 159
> > <https://reviews.apache.org/r/52476/diff/7/?file=1627958#file1627958line159>
> >
> > No comma before 'if' if it's the last clause in the sentence.
Fixed.
> On Feb. 8, 2017, 10:06 p.m., Prateek Maheshwari wrote:
> > samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala,
> > line 113
> > <https://reviews.apache.org/r/52476/diff/7/?file=1627958#file1627958line113>
> >
> > s/of the store/for the store.
Fixed.
> On Feb. 8, 2017, 10:06 p.m., Prateek Maheshwari wrote:
> > samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala,
> > line 120
> > <https://reviews.apache.org/r/52476/diff/7/?file=1627958#file1627958line120>
> >
> > @param doesn't go in the method description, use {@code}
Fixed.
> On Feb. 8, 2017, 10:06 p.m., Prateek Maheshwari wrote:
> > samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala,
> > line 135
> > <https://reviews.apache.org/r/52476/diff/7/?file=1627958#file1627958line135>
> >
> > Explain what stale means here.
> >
> > Use @code instead of @param here.
Fixed.
- Shanthoosh
-------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52476/#review164786
---
On Feb. 8, 2017, 9:37 p.m., Shanthoosh Venkataraman wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52476/
> ---
>
> (Updated Feb. 8, 2017, 9:37 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> ---
>
> Every local task store is backed up by a kafka changelog topic. Due to log
> compaction, delete tombstones of the changelog topic have a ttl of
> delete.retention.ms. Replaying the events from the changelog that has missing
> delete tombstones, would result in creation of an inconsistent local
> store(due to the missing of some delete events). This patch deletes the local
> stores in which difference between current time and last modified time of the
> offset file is greater than delete.retention.ms during the container startup.
>
>
> Diffs
> -
>
> samza-core/src/main/java/org/apache/samza/storage/StorageRecovery.java
> 9329edf7d724f3a0d9235354bb77936f713e3b5f
> samza-core/src/main/scala/org/apache/samza/config/StorageConfig.scala
> a3587d0a40c57374ee1742234929d444e381e42d
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala
> c3308bfd7de04c335fef6cb66baa29286a230080
> samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala
> 0b7bcdda1639eea8239a69c31bdf42558e9077d2
>
> samza-core/src/test/scala/org/apache/samza/storage/TestTaskStorageManager.scala
> 4d40f520e54beb643acd8410c772b75e2f6a9162
> samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala
> 9320cf744ff90d647a198b51cb06d2a526fe68fa
>
> Diff: https://reviews.apache.org/r/52476/diff/
>
>
> Testing
> ---
>
> Unit testing and manual testing has been done to verify the functionality.
>
>
> Thanks,
>
> Shanthoosh Venkataraman
>
>
Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job
/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382 samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089 samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797 samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION Diff: https://reviews.apache.org/r/52168/diff/ Testing --- Manual and unit testing has been done to verify the apis. Thanks, Shanthoosh Venkataraman
