Mailing-List: contact dev-help@falcon.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@falcon.incubator.apache.org
Content-Type: multipart/alternative;
 boundary="===============4686229613320903064=="
MIME-Version: 1.0
Subject: Re: Review Request 26908: FALCON-805
From: "Ajay Yadava" <ajaynsit@gmail.com>
To: "Srikanth Sundarrajan" <sriksun@hotmail.com>
Cc: "Falcon" <dev@falcon.incubator.apache.org>,
 "Ajay Yadava" <ajaynsit@gmail.com>
Date: Wed, 05 Nov 2014 04:30:22 -0000
Message-ID: <20141105043022.10454.13086@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Ajay Yadava" <noreply@reviews.apache.org>
References: <20141105012517.10455.92372@reviews.apache.org>
In-Reply-To: <20141105012517.10455.92372@reviews.apache.org>
Reply-To: "Ajay Yadava" <ajaynsit@gmail.com>

--===============4686229613320903064==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


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


--===============4686229613320903064==--