----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52492/#review152039 -----------------------------------------------------------
Initial scan. Will need to take another pass afther the below is addressed. samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java (lines 27 - 33) <https://reviews.apache.org/r/52492/#comment220742> Based on these dependencies, this class is not a generic LocalStoreMonitor. It should be made generic. If that's not possible, then it should be at least renamed to YarnLocalStoreMonitor. samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java (lines 47 - 52) <https://reviews.apache.org/r/52492/#comment220737> https://docs.oracle.com/javase/8/docs/technotes/guides/language/static-import.html "So when should you use static import? Very sparingly!" samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java (line 84) <https://reviews.apache.org/r/52492/#comment220743> Shouldn't this be checked only once in the constructor? samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java (line 114) <https://reviews.apache.org/r/52492/#comment220750> This is not an accurate name. Jobs can be stateful and not have any on disk state. Jobs can be stateful and not have any state in the logged stores directory. I'd recommend using similar terminology as is used in TaskStorageManager. It's accurate and we should use consistent terminology anyway. samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java (line 164) <https://reviews.apache.org/r/52492/#comment220758> 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 samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java (line 166) <https://reviews.apache.org/r/52492/#comment220741> This probably belongs in TasksResource or a TasksResourceConstants class. Declaring it here makes this class fragile to changes/evolutions in the Tasks API. samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java (line 170) <https://reviews.apache.org/r/52492/#comment220740> This probably belongs in JobsResource or a JobsResourceConstants class. Declaring it here makes this class fragile to changes/evolutions in the Jobs API. samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitorFactory.java (line 23) <https://reviews.apache.org/r/52492/#comment220738> https://docs.oracle.com/javase/8/docs/technotes/guides/language/static-import.html "So when should you use static import? Very sparingly!" samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java (line 30) <https://reviews.apache.org/r/52492/#comment220735> This class is introduced in https://reviews.apache.org/r/52168 Why is it duplicated here? samza-rest/src/main/java/org/apache/samza/rest/model/Task.java (line 30) <https://reviews.apache.org/r/52492/#comment220734> This class is introduced in https://reviews.apache.org/r/52168 Why is it duplicated here? samza-rest/src/test/java/org/apache/samza/monitor/TestLocalStoreMonitor.java (line 38) <https://reviews.apache.org/r/52492/#comment220739> https://docs.oracle.com/javase/8/docs/technotes/guides/language/static-import.html "So when should you use static import? Very sparingly!" - Jake Maes 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 > >
