----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19794/#review41921 -----------------------------------------------------------
Hi there! (I'm new to ZK, and I'm checking out some code reviews to get acquainted with the project.) The unit test here is good, but it doesn't appear to check that the code selects the correct set of files to purge, but that the process at least completes. It would be good to break out the logic for selecting the files into a separate method and then testing that with mock data for the variety of snapshot and log files available. It'd be easy to test edge cases that way, too. I see that Mockito is already a ZK dependency, and that can definitely do it. I'd be happy to help out with creating such a test. - Bill Havanki On March 29, 2014, 3:23 a.m., Rakesh R wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19794/ > ----------------------------------------------------------- > > (Updated March 29, 2014, 3:23 a.m.) > > > Review request for zookeeper, fpj, michim, Raul Gutierrez Segales, and > Camille Fournier. > > > Bugs: ZOOKEEPER-1797 > https://issues.apache.org/jira/browse/ZOOKEEPER-1797 > > > Repository: zookeeper > > > Description > ------- > > PurgeTxnLog may delete data logs if logs are rolling or a new snapshot is > created during this process > > > Diffs > ----- > > ./src/java/main/org/apache/zookeeper/server/PurgeTxnLog.java 1582962 > ./src/java/test/org/apache/zookeeper/test/PurgeTxnTest.java 1582962 > > Diff: https://reviews.apache.org/r/19794/diff/ > > > Testing > ------- > > I've tried to add a tests > > > Thanks, > > Rakesh R > >
