You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@falcon.apache.org by Srikanth Sundarrajan <sr...@hotmail.com> on 2014/11/05 02:25:17 UTC

Re: Review Request 26908: FALCON-805

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



common/src/main/java/org/apache/falcon/entity/store/FeedLocationStore.java
<https://reviews.apache.org/r/26908/#comment101214>

    An example to illustrate what is being stored, who help reader understand this more easily



common/src/main/java/org/apache/falcon/entity/store/FeedLocationStore.java
<https://reviews.apache.org/r/26908/#comment101213>

    Not final ?



common/src/main/java/org/apache/falcon/entity/store/FeedLocationStore.java
<https://reviews.apache.org/r/26908/#comment101216>

    final ?



common/src/main/java/org/apache/falcon/entity/store/FeedLocationStore.java
<https://reviews.apache.org/r/26908/#comment101215>

    final ?



common/src/main/java/org/apache/falcon/entity/store/FeedLocationStore.java
<https://reviews.apache.org/r/26908/#comment101217>

    final ?



common/src/main/java/org/apache/falcon/entity/store/FeedLocationStore.java
<https://reviews.apache.org/r/26908/#comment101218>

    Do we need a corresponding thing for Catalog based feeds, can be done in an independent JIRA, but do we see utility ?



common/src/main/java/org/apache/falcon/entity/store/FeedPathStore.java
<https://reviews.apache.org/r/26908/#comment101219>

    Since you are using javax.annotation, why not annotate for the arguments for the methods and describe whether they are nullable or nonnull?



common/src/main/java/org/apache/falcon/util/RadixNode.java
<https://reviews.apache.org/r/26908/#comment101220>

    Please return an unmodifiableCollection



common/src/main/java/org/apache/falcon/util/RadixTree.java
<https://reviews.apache.org/r/26908/#comment101222>

    This seems very critical method, will go through this to check for boundary conditions



common/src/main/java/org/apache/falcon/util/RadixTree.java
<https://reviews.apache.org/r/26908/#comment101221>

    There should be a behavior in RadixNode::removeAll



common/src/main/java/org/apache/falcon/util/RadixTree.java
<https://reviews.apache.org/r/26908/#comment101224>

    When does this happen ?



common/src/main/java/org/apache/falcon/util/RadixTree.java
<https://reviews.apache.org/r/26908/#comment101225>

    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 ?



common/src/main/java/org/apache/falcon/util/RadixTree.java
<https://reviews.apache.org/r/26908/#comment101226>

    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.


- Srikanth Sundarrajan


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

Re: Review Request 26908: FALCON-805

Posted by Ajay Yadava <aj...@gmail.com>.


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

Re: Review Request 26908: FALCON-805

Posted by Ajay Yadava <aj...@gmail.com>.


> 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
> 
> Ajay Yadava wrote:
>     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?

Fixed it.


- Ajay


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


On Nov. 10, 2014, 3:18 p.m., Ajay Yadava wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26908/
> -----------------------------------------------------------
> 
> (Updated Nov. 10, 2014, 3:18 p.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
> 
>