> 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? > > Shanthoosh Venkataraman wrote: > 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.
Either way the user needs to see the exception in the log. In one case it's a single exception and in the other it creates so much noise you can't see other problems in the log. I prefer the former. - Jake ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52492/#review152039 ----------------------------------------------------------- On Oct. 11, 2016, 12:53 a.m., Shanthoosh Venkataraman wrote: > > ----------------------------------------------------------- > 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 > ----- > > 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 > >
