----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52492/#review153772 -----------------------------------------------------------
samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java (line 107) <https://reviews.apache.org/r/52492/#comment223182> IMO, this can be made info. samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java (line 157) <https://reviews.apache.org/r/52492/#comment223183> Print what the config file TTL is. Also log what the last modified timestamp of the offset file is. samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java (line 166) <https://reviews.apache.org/r/52492/#comment223192> 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? samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java (line 171) <https://reviews.apache.org/r/52492/#comment223184> Do we update this list? If not, do you think it can be made final? Immutability is a nice property to have in general. samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java (line 186) <https://reviews.apache.org/r/52492/#comment223187> seems that this is just throwing a SamzaException. samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java (line 197) <https://reviews.apache.org/r/52492/#comment223186> Where is the exception being thrown? Is this declared with `throws`? samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java (line 215) <https://reviews.apache.org/r/52492/#comment223185> nit: Docs should accurate the type of exception being thrown. Would only RM unavailable throw this exception? It seems to me that any error in reading from stream will also throw it? samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitorConfig.java (line 49) <https://reviews.apache.org/r/52492/#comment223189> Are these acceseed outside? If so, can't these just be private? The same applies to all above variables. samza-rest/src/main/java/org/apache/samza/rest/resources/ResourceConstants.java (line 23) <https://reviews.apache.org/r/52492/#comment223188> Why do we need this class to store 2 global constants? Are these used elsewhere? They only seem to be used inside jobs client? It seems that they can be made private variables of that class? - Jagadish Venkatraman On Oct. 25, 2016, 8:14 p.m., Shanthoosh Venkataraman wrote: > > ----------------------------------------------------------- > 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 > ----- > > 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 > >
