> On April 10, 2015, 5:25 a.m., Pallavi Rao wrote: > > retention/src/test/java/org/apache/falcon/retention/FeedEvictorTest.java, > > line 392 > > <https://reviews.apache.org/r/33025/diff/1/?file=921767#file921767line392> > > > > The code fix is fine. But, don't think this test tests the patch > > sufficiently. If the retention limit is 10 days, nothing should ideally get > > deleted as we produce only 10 instances of data. > > Peeyush Bishnoi wrote: > I agree that nothing should ideally get deleted if retention limit is 10 > days with 10 instances. But in this test case we have more than 10 instances > thats why only 10 days data has been retained after eviction. > $ ls > retention/webapp/target/tmp-hadoop-pbishnoi/jail-fs/test/data/YYYY/feed1/mmHH/dd/MM/2015/04/ > 01 02 03 04 05 06 07 08 09 > 10 > > Also, here we have just re-enabled the testcase > testEvictionWithEmptyDirs(). > > Pallavi Rao wrote: > I understand this test was just re-enabled. We should either modify this > test or add a new one to specifically test your patch. Since not all > instances get removed, the parent dirs removal code path won't get tested. > > Sowmya Ramesh wrote: > Agree with Pallavi. Using these magic numbers in tests is not readable. > Test should be modified so that after eviction there will be no content under > feed base path dir. > > Peeyush Bishnoi wrote: > Already developed the testcase for this scenario. Will upload.
New test case added for this code fix that ensure feed base path must exist after all the instance directories deleted. > On April 10, 2015, 5:25 a.m., Pallavi Rao wrote: > > retention/src/test/java/org/apache/falcon/retention/FeedEvictorTest.java, > > line 410 > > <https://reviews.apache.org/r/33025/diff/1/?file=921767#file921767line410> > > > > You will notice that this check will pass even without your code fix > > (with just the change in line 401), coz., no instances really get evicted > > in this test. > > Peeyush Bishnoi wrote: > I have checked the logs and found that few instances has been evicted > through this testcase. As I mentioned, we have just re-enabled the test case. New test case added for this code fix that ensure feed base path must exist after all the instance directories deleted. > On April 10, 2015, 5:25 a.m., Pallavi Rao wrote: > > retention/src/test/java/org/apache/falcon/retention/FeedEvictorTest.java, > > line 412 > > <https://reviews.apache.org/r/33025/diff/1/?file=921767#file921767line412> > > > > If we delete all instances and only the basePath is left behind, it > > should have no children. So, afterDelCount should be zero. > > Peeyush Bishnoi wrote: > For this test case as the retention limit is 10 days , 10 days data is > available with total directory count 32 as few older instances evicted. > $ ls > retention/webapp/target/tmp-hadoop-pbishnoi/jail-fs/test/data/YYYY/feed1/mmHH/dd/MM/2015/04/ > 01 02 03 04 05 06 07 08 09 > 10 > $ ls -Rlt > retention/webapp/target/tmp-hadoop-pbishnoi/jail-fs/test/data/YYYY/feed1/mmHH/dd/MM/|grep > YYYY|wc -l > 32 New test case added for this code fix that ensure feed base path must exist after all the instance directories deleted. - Peeyush ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33025/#review79641 ----------------------------------------------------------- On April 13, 2015, 5:27 a.m., Peeyush Bishnoi wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33025/ > ----------------------------------------------------------- > > (Updated April 13, 2015, 5:27 a.m.) > > > Review request for Falcon. > > > Bugs: FALCON-1146 > https://issues.apache.org/jira/browse/FALCON-1146 > > > Repository: falcon-git > > > Description > ------- > > FALCON-1146 : feed retention policy deleted everything all the way up to the > root > > > Diffs > ----- > > common/src/main/java/org/apache/falcon/entity/FileSystemStorage.java > 8948ad4 > retention/src/test/java/org/apache/falcon/retention/FeedEvictorTest.java > a2feccf > > Diff: https://reviews.apache.org/r/33025/diff/ > > > Testing > ------- > > Yes > > > Thanks, > > Peeyush Bishnoi > >
