Mailing-List: contact dev-help@falcon.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@falcon.apache.org
Content-Type: multipart/alternative;
 boundary="===============3521480988872421011=="
MIME-Version: 1.0
Subject: Re: Review Request 38834: Refactor FalconCLI to make it more
 manageable.
From: "Balu Vellanki" <bvellanki@hortonworks.com>
To: "Sowmya Ramesh" <sramesh@hortonworks.com>,
 "Ajay Yadava" <ajaynsit@gmail.com>
Cc: "Balu Vellanki" <bvellanki@hortonworks.com>,
 "Falcon" <dev@falcon.incubator.apache.org>
Date: Tue, 29 Sep 2015 18:51:26 -0000
Message-ID: <20150929185126.23563.97149@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Balu Vellanki" <noreply@reviews.apache.org>
References: <20150929174632.23563.99907@reviews.apache.org>
In-Reply-To: <20150929174632.23563.99907@reviews.apache.org>
Reply-To: "Balu Vellanki" <bvellanki@hortonworks.com>

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


> On Sept. 29, 2015, 5:46 p.m., Ajay Yadava wrote:
> > client/src/main/java/org/apache/falcon/cli/FalconAdminCLI.java, line 37
> > <https://reviews.apache.org/r/38834/diff/1/?file=1086309#file1086309line37>
> >
> >     shouldn't version also be here? Is it used elsewhere also and you need it in base class?

Version is used in baseclass. So I need to keep it in FalconCLI


> On Sept. 29, 2015, 5:46 p.m., Ajay Yadava wrote:
> > client/src/main/java/org/apache/falcon/cli/FalconAdminCLI.java, line 38
> > <https://reviews.apache.org/r/38834/diff/1/?file=1086309#file1086309line38>
> >
> >     These options were earlier public and I am not sure but they might be used by regression etc. Since they are final, no harm in exposing them I guess.

I looked at Falcon regression and confirmed that they are not being used in Falcon regression code.


> On Sept. 29, 2015, 5:46 p.m., Ajay Yadava wrote:
> > client/src/main/java/org/apache/falcon/cli/FalconAdminCLI.java, line 58
> > <https://reviews.apache.org/r/38834/diff/1/?file=1086309#file1086309line58>
> >
> >     Should it be else if instead of if?

STACK_OPTION can be used along with other options. So it should not be "else if"


> On Sept. 29, 2015, 5:46 p.m., Ajay Yadava wrote:
> > client/src/main/java/org/apache/falcon/cli/FalconCLI.java, line 218
> > <https://reviews.apache.org/r/38834/diff/1/?file=1086310#file1086310line218>
> >
> >     Any reason for not returning the exit value for other commands?

entityCommand, instanceCommand, metadataCommand and recipeCommand return void. Hence we are not returning the exit value.


> On Sept. 29, 2015, 5:46 p.m., Ajay Yadava wrote:
> > client/src/main/java/org/apache/falcon/cli/FalconInstanceCLI.java, line 288
> > <https://reviews.apache.org/r/38834/diff/1/?file=1086312#file1086312line288>
> >
> >     Nit: Moving it up at the beginning will improve the readability.

Agreed, made all changes in Falcon*CLI


- Balu


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


On Sept. 29, 2015, 1 a.m., Balu Vellanki wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38834/
> -----------------------------------------------------------
> 
> (Updated Sept. 29, 2015, 1 a.m.)
> 
> 
> Review request for Falcon, Ajay Yadava and Sowmya Ramesh.
> 
> 
> Bugs: FALCON-592
>     https://issues.apache.org/jira/browse/FALCON-592
> 
> 
> Repository: falcon-git
> 
> 
> Description
> -------
> 
> Today, FalconCLI has options for all types of commands, namely instance, entity, admin, graph and recipe. When I had to write CLI for falcon Metadata, I separated it out into FalconMetadataCLI. This helped code become more readable and manageable.
> In similar fashion, break up FalconCLI into FalconInstanceCLI, FalconEntityCLI and so on.
> 
> 
> Diffs
> -----
> 
>   client/src/main/java/org/apache/falcon/cli/FalconAdminCLI.java PRE-CREATION 
>   client/src/main/java/org/apache/falcon/cli/FalconCLI.java c914649 
>   client/src/main/java/org/apache/falcon/cli/FalconEntityCLI.java PRE-CREATION 
>   client/src/main/java/org/apache/falcon/cli/FalconInstanceCLI.java PRE-CREATION 
>   client/src/main/java/org/apache/falcon/cli/FalconMetadataCLI.java dbc553c 
>   client/src/main/java/org/apache/falcon/cli/FalconRecipeCLI.java PRE-CREATION 
>   client/src/main/java/org/apache/falcon/recipe/RecipeFactory.java 32b0871 
>   webapp/src/test/java/org/apache/falcon/cli/FalconCLIIT.java 07aacdd 
> 
> Diff: https://reviews.apache.org/r/38834/diff/
> 
> 
> Testing
> -------
> 
> Unit and integration tests passed. Tested end2end with simple entity submission from CLI
> 
> 
> Thanks,
> 
> Balu Vellanki
> 
>


--===============3521480988872421011==--