You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@falcon.apache.org by Ajay Yadava <aj...@gmail.com> on 2014/08/30 09:21:57 UTC
Review Request 25211: Delegated feed eviction to the appropriate
Storage implementation
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25211/
-----------------------------------------------------------
Review request for Falcon and shwethags.
Repository: falcon-git
Description
-------
Earlier FeedEviction class contained logic for both FileSystemStorage and TableStorage. Corresponding code was being called using an if else. To make the code cleaner and more manageable I moved the code to the appropriate storage class and delegated feed eviction to the appropriate Storage implementation. Needed to add evict method to Storage Interface and make some minor changes here and there.
Diffs
-----
common/src/main/java/org/apache/falcon/entity/CatalogStorage.java 7ad0716
common/src/main/java/org/apache/falcon/entity/FileSystemStorage.java 4eb3d60
common/src/main/java/org/apache/falcon/entity/Storage.java f88e139
retention/src/main/java/org/apache/falcon/retention/FeedEvictor.java 4de7938
Diff: https://reviews.apache.org/r/25211/diff/
Testing
-------
All FeedEvictor tests passed.
Thanks,
Ajay Yadava
Re: Review Request 25211: Delegated feed eviction to the appropriate
Storage implementation
Posted by Ajay Yadava <aj...@gmail.com>.
> On Sept. 1, 2014, 6:28 a.m., shwethags wrote:
> > common/src/main/java/org/apache/falcon/entity/Storage.java, line 98
> > <https://reviews.apache.org/r/25211/diff/1/?file=672661#file672661line98>
> >
> > shoudl throw only FalconException
Will make the necessary changes and submit a new patch.
- Ajay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25211/#review51970
-----------------------------------------------------------
On Aug. 30, 2014, 7:22 a.m., Ajay Yadava wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25211/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2014, 7:22 a.m.)
>
>
> Review request for Falcon and shwethags.
>
>
> Repository: falcon-git
>
>
> Description
> -------
>
> Earlier FeedEviction class contained logic for both FileSystemStorage and TableStorage. Corresponding code was being called using an if else. To make the code cleaner and more manageable I moved the code to the appropriate storage class and delegated feed eviction to the appropriate Storage implementation. Needed to add evict method to Storage Interface and make some minor changes here and there.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/falcon/entity/CatalogStorage.java 7ad0716
> common/src/main/java/org/apache/falcon/entity/FileSystemStorage.java 4eb3d60
> common/src/main/java/org/apache/falcon/entity/Storage.java f88e139
> retention/src/main/java/org/apache/falcon/retention/FeedEvictor.java 4de7938
>
> Diff: https://reviews.apache.org/r/25211/diff/
>
>
> Testing
> -------
>
> All FeedEvictor tests passed.
>
>
> Thanks,
>
> Ajay Yadava
>
>
Re: Review Request 25211: Delegated feed eviction to the appropriate
Storage implementation
Posted by Ajay Yadava <aj...@gmail.com>.
On Sept. 1, 2014, 6:28 a.m., Ajay Yadava wrote:
> > move the corresponding UTs as well
Good Point. I gave it some thought and observed that the UTs are around various aspects of feed eviction only and are intended to test FeedEvictor, with fileSystemStorage being used for storage, falcon-retention still seems more natural place. Let me know if you still want me to move it to falcon-common.
- Ajay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25211/#review51970
-----------------------------------------------------------
On Aug. 30, 2014, 7:22 a.m., Ajay Yadava wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25211/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2014, 7:22 a.m.)
>
>
> Review request for Falcon and shwethags.
>
>
> Repository: falcon-git
>
>
> Description
> -------
>
> Earlier FeedEviction class contained logic for both FileSystemStorage and TableStorage. Corresponding code was being called using an if else. To make the code cleaner and more manageable I moved the code to the appropriate storage class and delegated feed eviction to the appropriate Storage implementation. Needed to add evict method to Storage Interface and make some minor changes here and there.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/falcon/entity/CatalogStorage.java 7ad0716
> common/src/main/java/org/apache/falcon/entity/FileSystemStorage.java 4eb3d60
> common/src/main/java/org/apache/falcon/entity/Storage.java f88e139
> retention/src/main/java/org/apache/falcon/retention/FeedEvictor.java 4de7938
>
> Diff: https://reviews.apache.org/r/25211/diff/
>
>
> Testing
> -------
>
> All FeedEvictor tests passed.
>
>
> Thanks,
>
> Ajay Yadava
>
>
Re: Review Request 25211: Delegated feed eviction to the appropriate
Storage implementation
Posted by sh...@inmobi.com.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25211/#review51970
-----------------------------------------------------------
common/src/main/java/org/apache/falcon/entity/Storage.java
<https://reviews.apache.org/r/25211/#comment90691>
shoudl throw only FalconException
move the corresponding UTs as well
- shwethags
On Aug. 30, 2014, 7:22 a.m., Ajay Yadava wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25211/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2014, 7:22 a.m.)
>
>
> Review request for Falcon and shwethags.
>
>
> Repository: falcon-git
>
>
> Description
> -------
>
> Earlier FeedEviction class contained logic for both FileSystemStorage and TableStorage. Corresponding code was being called using an if else. To make the code cleaner and more manageable I moved the code to the appropriate storage class and delegated feed eviction to the appropriate Storage implementation. Needed to add evict method to Storage Interface and make some minor changes here and there.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/falcon/entity/CatalogStorage.java 7ad0716
> common/src/main/java/org/apache/falcon/entity/FileSystemStorage.java 4eb3d60
> common/src/main/java/org/apache/falcon/entity/Storage.java f88e139
> retention/src/main/java/org/apache/falcon/retention/FeedEvictor.java 4de7938
>
> Diff: https://reviews.apache.org/r/25211/diff/
>
>
> Testing
> -------
>
> All FeedEvictor tests passed.
>
>
> Thanks,
>
> Ajay Yadava
>
>
Re: Review Request 25211: Delegated feed eviction to the appropriate
Storage implementation
Posted by Srikanth Sundarrajan <sr...@hotmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25211/#review60096
-----------------------------------------------------------
Ship it!
Ship It!
- Srikanth Sundarrajan
On Nov. 4, 2014, 6:28 p.m., Ajay Yadava wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25211/
> -----------------------------------------------------------
>
> (Updated Nov. 4, 2014, 6:28 p.m.)
>
>
> Review request for Falcon and shwethags.
>
>
> Repository: falcon-git
>
>
> Description
> -------
>
> Earlier FeedEviction class contained logic for both FileSystemStorage and TableStorage. Corresponding code was being called using an if else. To make the code cleaner and more manageable I moved the code to the appropriate storage class and delegated feed eviction to the appropriate Storage implementation. Needed to add evict method to Storage Interface and make some minor changes here and there.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/falcon/entity/CatalogStorage.java dbb0293
> common/src/main/java/org/apache/falcon/entity/FeedHelper.java 4532669
> common/src/main/java/org/apache/falcon/entity/FileSystemStorage.java 012a6e7
> common/src/main/java/org/apache/falcon/entity/Storage.java af3040b
> common/src/main/java/org/apache/falcon/retention/EvictionHelper.java PRE-CREATION
> retention/src/main/java/org/apache/falcon/retention/FeedEvictor.java 67ea181
>
> Diff: https://reviews.apache.org/r/25211/diff/
>
>
> Testing
> -------
>
> All FeedEvictor tests passed.
>
>
> Thanks,
>
> Ajay Yadava
>
>
Re: Review Request 25211: Delegated feed eviction to the appropriate
Storage implementation
Posted by Ajay Yadava <aj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25211/
-----------------------------------------------------------
(Updated Nov. 4, 2014, 6:28 p.m.)
Review request for Falcon and shwethags.
Repository: falcon-git
Description
-------
Earlier FeedEviction class contained logic for both FileSystemStorage and TableStorage. Corresponding code was being called using an if else. To make the code cleaner and more manageable I moved the code to the appropriate storage class and delegated feed eviction to the appropriate Storage implementation. Needed to add evict method to Storage Interface and make some minor changes here and there.
Diffs (updated)
-----
common/src/main/java/org/apache/falcon/entity/CatalogStorage.java dbb0293
common/src/main/java/org/apache/falcon/entity/FeedHelper.java 4532669
common/src/main/java/org/apache/falcon/entity/FileSystemStorage.java 012a6e7
common/src/main/java/org/apache/falcon/entity/Storage.java af3040b
common/src/main/java/org/apache/falcon/retention/EvictionHelper.java PRE-CREATION
retention/src/main/java/org/apache/falcon/retention/FeedEvictor.java 67ea181
Diff: https://reviews.apache.org/r/25211/diff/
Testing
-------
All FeedEvictor tests passed.
Thanks,
Ajay Yadava
Re: Review Request 25211: Delegated feed eviction to the appropriate
Storage implementation
Posted by Srikanth Sundarrajan <sr...@hotmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25211/#review54525
-----------------------------------------------------------
Ship it!
Ship It!
- Srikanth Sundarrajan
On Sept. 16, 2014, 11:25 a.m., Ajay Yadava wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25211/
> -----------------------------------------------------------
>
> (Updated Sept. 16, 2014, 11:25 a.m.)
>
>
> Review request for Falcon and shwethags.
>
>
> Repository: falcon-git
>
>
> Description
> -------
>
> Earlier FeedEviction class contained logic for both FileSystemStorage and TableStorage. Corresponding code was being called using an if else. To make the code cleaner and more manageable I moved the code to the appropriate storage class and delegated feed eviction to the appropriate Storage implementation. Needed to add evict method to Storage Interface and make some minor changes here and there.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/falcon/entity/CatalogStorage.java 7ad0716
> common/src/main/java/org/apache/falcon/entity/FileSystemStorage.java 58506ad
> common/src/main/java/org/apache/falcon/entity/Storage.java f88e139
> common/src/main/java/org/apache/falcon/retention/EvictionHelper.java 5d6481c
> retention/src/main/java/org/apache/falcon/retention/FeedEvictor.java 114071f
>
> Diff: https://reviews.apache.org/r/25211/diff/
>
>
> Testing
> -------
>
> All FeedEvictor tests passed.
>
>
> Thanks,
>
> Ajay Yadava
>
>
Re: Review Request 25211: Delegated feed eviction to the appropriate
Storage implementation
Posted by Ajay Yadava <aj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25211/
-----------------------------------------------------------
(Updated Sept. 16, 2014, 11:25 a.m.)
Review request for Falcon and shwethags.
Changes
-------
Converted StringBuffer to StringBuilder
Repository: falcon-git
Description
-------
Earlier FeedEviction class contained logic for both FileSystemStorage and TableStorage. Corresponding code was being called using an if else. To make the code cleaner and more manageable I moved the code to the appropriate storage class and delegated feed eviction to the appropriate Storage implementation. Needed to add evict method to Storage Interface and make some minor changes here and there.
Diffs (updated)
-----
common/src/main/java/org/apache/falcon/entity/CatalogStorage.java 7ad0716
common/src/main/java/org/apache/falcon/entity/FileSystemStorage.java 58506ad
common/src/main/java/org/apache/falcon/entity/Storage.java f88e139
common/src/main/java/org/apache/falcon/retention/EvictionHelper.java 5d6481c
retention/src/main/java/org/apache/falcon/retention/FeedEvictor.java 114071f
Diff: https://reviews.apache.org/r/25211/diff/
Testing
-------
All FeedEvictor tests passed.
Thanks,
Ajay Yadava
Re: Review Request 25211: Delegated feed eviction to the appropriate
Storage implementation
Posted by Ajay Yadava <aj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25211/
-----------------------------------------------------------
(Updated Sept. 13, 2014, 4:43 a.m.)
Review request for Falcon and shwethags.
Changes
-------
Rebased the patch. Enumerated the imports.
Repository: falcon-git
Description
-------
Earlier FeedEviction class contained logic for both FileSystemStorage and TableStorage. Corresponding code was being called using an if else. To make the code cleaner and more manageable I moved the code to the appropriate storage class and delegated feed eviction to the appropriate Storage implementation. Needed to add evict method to Storage Interface and make some minor changes here and there.
Diffs (updated)
-----
common/src/main/java/org/apache/falcon/entity/CatalogStorage.java 7ad0716
common/src/main/java/org/apache/falcon/entity/FileSystemStorage.java 58506ad
common/src/main/java/org/apache/falcon/entity/Storage.java f88e139
common/src/main/java/org/apache/falcon/retention/EvictionHelper.java 5d6481c
retention/src/main/java/org/apache/falcon/retention/FeedEvictor.java 114071f
Diff: https://reviews.apache.org/r/25211/diff/
Testing
-------
All FeedEvictor tests passed.
Thanks,
Ajay Yadava
Re: Review Request 25211: Delegated feed eviction to the appropriate
Storage implementation
Posted by Ajay Yadava <aj...@gmail.com>.
> On Sept. 12, 2014, 7:20 a.m., Srikanth Sundarrajan wrote:
> > common/src/main/java/org/apache/falcon/entity/FileSystemStorage.java, line 299
> > <https://reviews.apache.org/r/25211/diff/3/?file=682132#file682132line299>
> >
> > Are you better off using StringBuilder?
Original code used StringBuffer, so I have retained it to be on safer side, in case it was required for some concurrency scenario. Let me know if I can safely change it to StringBuilder.
On Sept. 12, 2014, 7:20 a.m., Ajay Yadava wrote:
> > Haven't checked the actual evict logic. Can you confirm if it is merely moved from FeedEvictor into respective Storage classes or have they been modified as well ?
No Changes have been made to any evict or related logic. I have kept the code as close to original implementation as possible with moving it to appropriate storage classes.
- Ajay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25211/#review53155
-----------------------------------------------------------
On Sept. 13, 2014, 4:43 a.m., Ajay Yadava wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25211/
> -----------------------------------------------------------
>
> (Updated Sept. 13, 2014, 4:43 a.m.)
>
>
> Review request for Falcon and shwethags.
>
>
> Repository: falcon-git
>
>
> Description
> -------
>
> Earlier FeedEviction class contained logic for both FileSystemStorage and TableStorage. Corresponding code was being called using an if else. To make the code cleaner and more manageable I moved the code to the appropriate storage class and delegated feed eviction to the appropriate Storage implementation. Needed to add evict method to Storage Interface and make some minor changes here and there.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/falcon/entity/CatalogStorage.java 7ad0716
> common/src/main/java/org/apache/falcon/entity/FileSystemStorage.java 58506ad
> common/src/main/java/org/apache/falcon/entity/Storage.java f88e139
> common/src/main/java/org/apache/falcon/retention/EvictionHelper.java 5d6481c
> retention/src/main/java/org/apache/falcon/retention/FeedEvictor.java 114071f
>
> Diff: https://reviews.apache.org/r/25211/diff/
>
>
> Testing
> -------
>
> All FeedEvictor tests passed.
>
>
> Thanks,
>
> Ajay Yadava
>
>
Re: Review Request 25211: Delegated feed eviction to the appropriate
Storage implementation
Posted by Srikanth Sundarrajan <sr...@hotmail.com>.
> On Sept. 12, 2014, 7:20 a.m., Srikanth Sundarrajan wrote:
> > common/src/main/java/org/apache/falcon/entity/FileSystemStorage.java, line 299
> > <https://reviews.apache.org/r/25211/diff/3/?file=682132#file682132line299>
> >
> > Are you better off using StringBuilder?
>
> Ajay Yadava wrote:
> Original code used StringBuffer, so I have retained it to be on safer side, in case it was required for some concurrency scenario. Let me know if I can safely change it to StringBuilder.
I think it can be safely switched to StringBuilder. The ealier use may be unintentional.
On Sept. 12, 2014, 7:20 a.m., Ajay Yadava wrote:
> > Haven't checked the actual evict logic. Can you confirm if it is merely moved from FeedEvictor into respective Storage classes or have they been modified as well ?
>
> Ajay Yadava wrote:
> No Changes have been made to any evict or related logic. I have kept the code as close to original implementation as possible with moving it to appropriate storage classes.
OK
- Srikanth
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25211/#review53155
-----------------------------------------------------------
On Sept. 13, 2014, 4:43 a.m., Ajay Yadava wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25211/
> -----------------------------------------------------------
>
> (Updated Sept. 13, 2014, 4:43 a.m.)
>
>
> Review request for Falcon and shwethags.
>
>
> Repository: falcon-git
>
>
> Description
> -------
>
> Earlier FeedEviction class contained logic for both FileSystemStorage and TableStorage. Corresponding code was being called using an if else. To make the code cleaner and more manageable I moved the code to the appropriate storage class and delegated feed eviction to the appropriate Storage implementation. Needed to add evict method to Storage Interface and make some minor changes here and there.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/falcon/entity/CatalogStorage.java 7ad0716
> common/src/main/java/org/apache/falcon/entity/FileSystemStorage.java 58506ad
> common/src/main/java/org/apache/falcon/entity/Storage.java f88e139
> common/src/main/java/org/apache/falcon/retention/EvictionHelper.java 5d6481c
> retention/src/main/java/org/apache/falcon/retention/FeedEvictor.java 114071f
>
> Diff: https://reviews.apache.org/r/25211/diff/
>
>
> Testing
> -------
>
> All FeedEvictor tests passed.
>
>
> Thanks,
>
> Ajay Yadava
>
>
Re: Review Request 25211: Delegated feed eviction to the appropriate
Storage implementation
Posted by Srikanth Sundarrajan <sr...@hotmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25211/#review53155
-----------------------------------------------------------
common/src/main/java/org/apache/falcon/entity/CatalogStorage.java
<https://reviews.apache.org/r/25211/#comment92614>
Please enumerate the imports
common/src/main/java/org/apache/falcon/entity/FileSystemStorage.java
<https://reviews.apache.org/r/25211/#comment92615>
Are you better off using StringBuilder?
Haven't checked the actual evict logic. Can you confirm if it is merely moved from FeedEvictor into respective Storage classes or have they been modified as well ?
- Srikanth Sundarrajan
On Sept. 6, 2014, 8:41 p.m., Ajay Yadava wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25211/
> -----------------------------------------------------------
>
> (Updated Sept. 6, 2014, 8:41 p.m.)
>
>
> Review request for Falcon and shwethags.
>
>
> Repository: falcon-git
>
>
> Description
> -------
>
> Earlier FeedEviction class contained logic for both FileSystemStorage and TableStorage. Corresponding code was being called using an if else. To make the code cleaner and more manageable I moved the code to the appropriate storage class and delegated feed eviction to the appropriate Storage implementation. Needed to add evict method to Storage Interface and make some minor changes here and there.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/falcon/entity/CatalogStorage.java 7ad0716
> common/src/main/java/org/apache/falcon/entity/FileSystemStorage.java 4eb3d60
> common/src/main/java/org/apache/falcon/entity/Storage.java f88e139
> common/src/main/java/org/apache/falcon/retention/EvictionHelper.java 5d6481c
> retention/src/main/java/org/apache/falcon/retention/FeedEvictor.java 114071f
>
> Diff: https://reviews.apache.org/r/25211/diff/
>
>
> Testing
> -------
>
> All FeedEvictor tests passed.
>
>
> Thanks,
>
> Ajay Yadava
>
>
Re: Review Request 25211: Delegated feed eviction to the appropriate
Storage implementation
Posted by Ajay Yadava <aj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25211/
-----------------------------------------------------------
(Updated Sept. 6, 2014, 8:41 p.m.)
Review request for Falcon and shwethags.
Repository: falcon-git
Description
-------
Earlier FeedEviction class contained logic for both FileSystemStorage and TableStorage. Corresponding code was being called using an if else. To make the code cleaner and more manageable I moved the code to the appropriate storage class and delegated feed eviction to the appropriate Storage implementation. Needed to add evict method to Storage Interface and make some minor changes here and there.
Diffs (updated)
-----
common/src/main/java/org/apache/falcon/entity/CatalogStorage.java 7ad0716
common/src/main/java/org/apache/falcon/entity/FileSystemStorage.java 4eb3d60
common/src/main/java/org/apache/falcon/entity/Storage.java f88e139
common/src/main/java/org/apache/falcon/retention/EvictionHelper.java 5d6481c
retention/src/main/java/org/apache/falcon/retention/FeedEvictor.java 114071f
Diff: https://reviews.apache.org/r/25211/diff/
Testing
-------
All FeedEvictor tests passed.
Thanks,
Ajay Yadava
Re: Review Request 25211: Delegated feed eviction to the appropriate
Storage implementation
Posted by Ajay Yadava <aj...@gmail.com>.
> On Sept. 2, 2014, 4:57 a.m., shwethags wrote:
> > common/src/main/java/org/apache/falcon/entity/FileSystemStorage.java, line 584
> > <https://reviews.apache.org/r/25211/diff/2/?file=673443#file673443line584>
> >
> > What is OUT?
> >
> > You don't need conf. You should be able to get filesystem handle from the cluster name
As discussed offline earlier, this is required for tests.
- Ajay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25211/#review52014
-----------------------------------------------------------
On Sept. 1, 2014, 1:31 p.m., Ajay Yadava wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25211/
> -----------------------------------------------------------
>
> (Updated Sept. 1, 2014, 1:31 p.m.)
>
>
> Review request for Falcon and shwethags.
>
>
> Repository: falcon-git
>
>
> Description
> -------
>
> Earlier FeedEviction class contained logic for both FileSystemStorage and TableStorage. Corresponding code was being called using an if else. To make the code cleaner and more manageable I moved the code to the appropriate storage class and delegated feed eviction to the appropriate Storage implementation. Needed to add evict method to Storage Interface and make some minor changes here and there.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/falcon/entity/CatalogStorage.java 7ad0716
> common/src/main/java/org/apache/falcon/entity/FileSystemStorage.java 4eb3d60
> common/src/main/java/org/apache/falcon/entity/Storage.java f88e139
> retention/src/main/java/org/apache/falcon/retention/FeedEvictor.java 4de7938
> retention/src/test/java/org/apache/falcon/retention/FeedEvictorFileSystemTest.java PRE-CREATION
> retention/src/test/java/org/apache/falcon/retention/FeedEvictorTest.java eb4173e
>
> Diff: https://reviews.apache.org/r/25211/diff/
>
>
> Testing
> -------
>
> All FeedEvictor tests passed.
>
>
> Thanks,
>
> Ajay Yadava
>
>
Re: Review Request 25211: Delegated feed eviction to the appropriate
Storage implementation
Posted by Ajay Yadava <aj...@gmail.com>.
> On Sept. 2, 2014, 4:57 a.m., shwethags wrote:
> > common/src/main/java/org/apache/falcon/entity/Storage.java, line 102
> > <https://reviews.apache.org/r/25211/diff/2/?file=673444#file673444line102>
> >
> > should have args - retentionLimit and timezone. It can probably return back list of instances deleted. FeedEvictor class can then log these instances as its common to both catalog storage and filesystem storage
Your suggestion is completely correct and the way this code is written is slightly convoluted e.g. the OUT is being used by Tests (passed through feedevictor) As part of this JIRA I have just separated the concerns to respective storage systems and kept true to original implementation. I was planning to handle the refactoring of Tests and Evictor logic, in separate JIRA. Thoughts?
- Ajay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25211/#review52014
-----------------------------------------------------------
On Sept. 1, 2014, 1:31 p.m., Ajay Yadava wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25211/
> -----------------------------------------------------------
>
> (Updated Sept. 1, 2014, 1:31 p.m.)
>
>
> Review request for Falcon and shwethags.
>
>
> Repository: falcon-git
>
>
> Description
> -------
>
> Earlier FeedEviction class contained logic for both FileSystemStorage and TableStorage. Corresponding code was being called using an if else. To make the code cleaner and more manageable I moved the code to the appropriate storage class and delegated feed eviction to the appropriate Storage implementation. Needed to add evict method to Storage Interface and make some minor changes here and there.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/falcon/entity/CatalogStorage.java 7ad0716
> common/src/main/java/org/apache/falcon/entity/FileSystemStorage.java 4eb3d60
> common/src/main/java/org/apache/falcon/entity/Storage.java f88e139
> retention/src/main/java/org/apache/falcon/retention/FeedEvictor.java 4de7938
> retention/src/test/java/org/apache/falcon/retention/FeedEvictorFileSystemTest.java PRE-CREATION
> retention/src/test/java/org/apache/falcon/retention/FeedEvictorTest.java eb4173e
>
> Diff: https://reviews.apache.org/r/25211/diff/
>
>
> Testing
> -------
>
> All FeedEvictor tests passed.
>
>
> Thanks,
>
> Ajay Yadava
>
>
Re: Review Request 25211: Delegated feed eviction to the appropriate
Storage implementation
Posted by sh...@inmobi.com.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25211/#review52014
-----------------------------------------------------------
common/src/main/java/org/apache/falcon/entity/FileSystemStorage.java
<https://reviews.apache.org/r/25211/#comment90717>
What is OUT?
You don't need conf. You should be able to get filesystem handle from the cluster name
common/src/main/java/org/apache/falcon/entity/Storage.java
<https://reviews.apache.org/r/25211/#comment90718>
should have args - retentionLimit and timezone. It can probably return back list of instances deleted. FeedEvictor class can then log these instances as its common to both catalog storage and filesystem storage
- shwethags
On Sept. 1, 2014, 1:31 p.m., Ajay Yadava wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25211/
> -----------------------------------------------------------
>
> (Updated Sept. 1, 2014, 1:31 p.m.)
>
>
> Review request for Falcon and shwethags.
>
>
> Repository: falcon-git
>
>
> Description
> -------
>
> Earlier FeedEviction class contained logic for both FileSystemStorage and TableStorage. Corresponding code was being called using an if else. To make the code cleaner and more manageable I moved the code to the appropriate storage class and delegated feed eviction to the appropriate Storage implementation. Needed to add evict method to Storage Interface and make some minor changes here and there.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/falcon/entity/CatalogStorage.java 7ad0716
> common/src/main/java/org/apache/falcon/entity/FileSystemStorage.java 4eb3d60
> common/src/main/java/org/apache/falcon/entity/Storage.java f88e139
> retention/src/main/java/org/apache/falcon/retention/FeedEvictor.java 4de7938
> retention/src/test/java/org/apache/falcon/retention/FeedEvictorFileSystemTest.java PRE-CREATION
> retention/src/test/java/org/apache/falcon/retention/FeedEvictorTest.java eb4173e
>
> Diff: https://reviews.apache.org/r/25211/diff/
>
>
> Testing
> -------
>
> All FeedEvictor tests passed.
>
>
> Thanks,
>
> Ajay Yadava
>
>
Re: Review Request 25211: Delegated feed eviction to the appropriate
Storage implementation
Posted by Ajay Yadava <aj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25211/
-----------------------------------------------------------
(Updated Sept. 1, 2014, 1:31 p.m.)
Review request for Falcon and shwethags.
Changes
-------
Implemented code review comments
Repository: falcon-git
Description
-------
Earlier FeedEviction class contained logic for both FileSystemStorage and TableStorage. Corresponding code was being called using an if else. To make the code cleaner and more manageable I moved the code to the appropriate storage class and delegated feed eviction to the appropriate Storage implementation. Needed to add evict method to Storage Interface and make some minor changes here and there.
Diffs (updated)
-----
common/src/main/java/org/apache/falcon/entity/CatalogStorage.java 7ad0716
common/src/main/java/org/apache/falcon/entity/FileSystemStorage.java 4eb3d60
common/src/main/java/org/apache/falcon/entity/Storage.java f88e139
retention/src/main/java/org/apache/falcon/retention/FeedEvictor.java 4de7938
retention/src/test/java/org/apache/falcon/retention/FeedEvictorFileSystemTest.java PRE-CREATION
retention/src/test/java/org/apache/falcon/retention/FeedEvictorTest.java eb4173e
Diff: https://reviews.apache.org/r/25211/diff/
Testing
-------
All FeedEvictor tests passed.
Thanks,
Ajay Yadava
Re: Review Request 25211: Delegated feed eviction to the appropriate
Storage implementation
Posted by Ajay Yadava <aj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25211/
-----------------------------------------------------------
(Updated Aug. 30, 2014, 7:22 a.m.)
Review request for Falcon and shwethags.
Repository: falcon-git
Description
-------
Earlier FeedEviction class contained logic for both FileSystemStorage and TableStorage. Corresponding code was being called using an if else. To make the code cleaner and more manageable I moved the code to the appropriate storage class and delegated feed eviction to the appropriate Storage implementation. Needed to add evict method to Storage Interface and make some minor changes here and there.
Diffs
-----
common/src/main/java/org/apache/falcon/entity/CatalogStorage.java 7ad0716
common/src/main/java/org/apache/falcon/entity/FileSystemStorage.java 4eb3d60
common/src/main/java/org/apache/falcon/entity/Storage.java f88e139
retention/src/main/java/org/apache/falcon/retention/FeedEvictor.java 4de7938
Diff: https://reviews.apache.org/r/25211/diff/
Testing
-------
All FeedEvictor tests passed.
Thanks,
Ajay Yadava
Re: Review Request 25211: Delegated feed eviction to the appropriate
Storage implementation
Posted by Ajay Yadava <aj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25211/
-----------------------------------------------------------
(Updated Aug. 30, 2014, 7:22 a.m.)
Review request for Falcon and shwethags.
Repository: falcon-git
Description
-------
Earlier FeedEviction class contained logic for both FileSystemStorage and TableStorage. Corresponding code was being called using an if else. To make the code cleaner and more manageable I moved the code to the appropriate storage class and delegated feed eviction to the appropriate Storage implementation. Needed to add evict method to Storage Interface and make some minor changes here and there.
Diffs
-----
common/src/main/java/org/apache/falcon/entity/CatalogStorage.java 7ad0716
common/src/main/java/org/apache/falcon/entity/FileSystemStorage.java 4eb3d60
common/src/main/java/org/apache/falcon/entity/Storage.java f88e139
retention/src/main/java/org/apache/falcon/retention/FeedEvictor.java 4de7938
Diff: https://reviews.apache.org/r/25211/diff/
Testing
-------
All FeedEvictor tests passed.
Thanks,
Ajay Yadava