falcon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Peeyush Bishnoi" <bpeey...@yahoo.co.in>
Subject Re: Review Request 33025: FALCON-1146 : feed retention policy deleted everything all the way up to the root
Date Mon, 13 Apr 2015 05:45:00 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message