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