falcon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ajay Yadava" <ajayn...@gmail.com>
Subject Re: Review Request 26908: FALCON-805
Date Wed, 05 Nov 2014 04:30:22 GMT


> On Nov. 5, 2014, 1:25 a.m., Srikanth Sundarrajan wrote:
> > common/src/main/java/org/apache/falcon/entity/store/FeedLocationStore.java, line
115
> > <https://reviews.apache.org/r/26908/diff/1/?file=725327#file725327line115>
> >
> >     Do we need a corresponding thing for Catalog based feeds, can be done in an
independent JIRA, but do we see utility ?

There is already a corresponding subtask for the same in the parent JIRA (FALCON-265) to investigate
the same and make changes if needed.


> On Nov. 5, 2014, 1:25 a.m., Srikanth Sundarrajan wrote:
> > common/src/main/java/org/apache/falcon/util/RadixTree.java, line 198
> > <https://reviews.apache.org/r/26908/diff/1/?file=725330#file725330line198>
> >
> >     When does this happen ?

when you have two paths say abcdef and abcghi

abc
  -->def
  -->ghi

and you search for abc.


> On Nov. 5, 2014, 1:25 a.m., Srikanth Sundarrajan wrote:
> > common/src/main/java/org/apache/falcon/util/RadixTree.java, line 207
> > <https://reviews.apache.org/r/26908/diff/1/?file=725330#file725330line207>
> >
> >     Not sure if I understand this. Can we run through and find what happens if the
tree has
> >     
> >     root-->aaa
> >         -->aab
> >         -->aac
> >         -
> >     
> >     and we search for root/aab/xyz, would it navigate into root/aaa and abort without
finding the element ?

In the example provided by you there is no path root/aab/xyz in the tree, so it should and
will abort without finding the element. Did I miss something?


> On Nov. 5, 2014, 1:25 a.m., Srikanth Sundarrajan wrote:
> > common/src/main/java/org/apache/falcon/util/RadixTree.java, line 87
> > <https://reviews.apache.org/r/26908/diff/1/?file=725330#file725330line87>
> >
> >     This seems very critical method, will go through this to check for boundary
conditions

Please go through test cases, I tried to make them very thorough and incorporate boundary
conditions. If you find some boundary condition missing, please let me know and I will add
a test for the same.


> On Nov. 5, 2014, 1:25 a.m., Srikanth Sundarrajan wrote:
> > common/src/main/java/org/apache/falcon/util/RadixTree.java, line 237
> > <https://reviews.apache.org/r/26908/diff/1/?file=725330#file725330line237>
> >
> >     Can the find be implemented in such a way that both insert & remove can
use, Currently each of these functions are slight variants of each other and bugs have to
be fixed in each one of them.

Good observation. That's how I wrote it initially but there are some differences which make
it slightly tricky, e.g. we stop at parent in delete to do compaction, if needed. Such small
differences make it tricky. I will give it another try.


> On Nov. 5, 2014, 1:25 a.m., Srikanth Sundarrajan wrote:
> > common/src/main/java/org/apache/falcon/util/RadixTree.java, line 127
> > <https://reviews.apache.org/r/26908/diff/1/?file=725330#file725330line127>
> >
> >     There should be a behavior in RadixNode::removeAll

Absolutely. How did I miss that!


> On Nov. 5, 2014, 1:25 a.m., Srikanth Sundarrajan wrote:
> > common/src/main/java/org/apache/falcon/util/RadixNode.java, line 77
> > <https://reviews.apache.org/r/26908/diff/1/?file=725329#file725329line77>
> >
> >     Please return an unmodifiableCollection

I am returning an unmodifiable collection from the implementation. Collections.unmodifiableCollection
returns a Collection. Are you suggesting me to use UnmodifiableCollection interface from some
other library like commons?


- Ajay


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26908/#review59897
-----------------------------------------------------------


On Oct. 19, 2014, 4:20 a.m., Ajay Yadava wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26908/
> -----------------------------------------------------------
> 
> (Updated Oct. 19, 2014, 4:20 a.m.)
> 
> 
> Review request for Falcon and Srikanth Sundarrajan.
> 
> 
> Repository: falcon-git
> 
> 
> Description
> -------
> 
> Store for feed path(key) and feed properties like feed name etc. as values
> 
> 
> Diffs
> -----
> 
>   common/src/main/java/org/apache/falcon/entity/store/FeedLocationStore.java PRE-CREATION

>   common/src/main/java/org/apache/falcon/entity/store/FeedPathStore.java PRE-CREATION

>   common/src/main/java/org/apache/falcon/util/RadixNode.java PRE-CREATION 
>   common/src/main/java/org/apache/falcon/util/RadixTree.java PRE-CREATION 
>   common/src/test/java/org/apache/falcon/entity/store/FeedLocationStoreTest.java PRE-CREATION

>   common/src/test/java/org/apache/falcon/util/RadixNodeTest.java PRE-CREATION 
>   common/src/test/java/org/apache/falcon/util/RadixTreeTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/26908/diff/
> 
> 
> Testing
> -------
> 
> Thorough unit testing for the patch submitted.
> 
> 
> Thanks,
> 
> Ajay Yadava
> 
>


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