You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@falcon.apache.org by Balu Vellanki <bv...@hortonworks.com> on 2015/09/29 03:00:43 UTC
Review Request 38834: Refactor FalconCLI to make it more manageable.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38834/
-----------------------------------------------------------
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
Re: Review Request 38834: Refactor FalconCLI to make it more
manageable.
Posted by Balu Vellanki <bv...@hortonworks.com>.
> 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
>
>
Re: Review Request 38834: Refactor FalconCLI to make it more
manageable.
Posted by Ajay Yadava <aj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38834/#review100976
-----------------------------------------------------------
I really love this JIRA as it will make the code a lot more manageable. Patch needs rebase. I assume you have just moved the code and not changed anything else. Can you please confirm?
client/src/main/java/org/apache/falcon/cli/FalconAdminCLI.java (line 37)
<https://reviews.apache.org/r/38834/#comment158266>
shouldn't version also be here? Is it used elsewhere also and you need it in base class?
client/src/main/java/org/apache/falcon/cli/FalconAdminCLI.java (line 38)
<https://reviews.apache.org/r/38834/#comment158267>
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.
client/src/main/java/org/apache/falcon/cli/FalconAdminCLI.java (line 58)
<https://reviews.apache.org/r/38834/#comment158268>
Should it be else if instead of if?
client/src/main/java/org/apache/falcon/cli/FalconCLI.java (line 155)
<https://reviews.apache.org/r/38834/#comment158269>
Any reason for not returning the exit value for other commands?
client/src/main/java/org/apache/falcon/cli/FalconInstanceCLI.java (line 288)
<https://reviews.apache.org/r/38834/#comment158272>
Nit: Moving it up at the beginning will improve the readability.
- Ajay Yadava
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
>
>
Re: Review Request 38834: Refactor FalconCLI to make it more
manageable.
Posted by sandeep samudrala <sa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38834/#review101070
-----------------------------------------------------------
Ship it!
Ship It!
- sandeep samudrala
On Sept. 29, 2015, 7:09 p.m., Balu Vellanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38834/
> -----------------------------------------------------------
>
> (Updated Sept. 29, 2015, 7:09 p.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 1574585
> 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
> 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
>
>
Re: Review Request 38834: Refactor FalconCLI to make it more
manageable.
Posted by Ajay Yadava <aj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38834/#review101081
-----------------------------------------------------------
Ship it!
Ship It!
- Ajay Yadava
On Sept. 29, 2015, 7:09 p.m., Balu Vellanki wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38834/
> -----------------------------------------------------------
>
> (Updated Sept. 29, 2015, 7:09 p.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 1574585
> 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
> 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
>
>
Re: Review Request 38834: Refactor FalconCLI to make it more
manageable.
Posted by Balu Vellanki <bv...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38834/
-----------------------------------------------------------
(Updated Sept. 29, 2015, 7:09 p.m.)
Review request for Falcon, Ajay Yadava and Sowmya Ramesh.
Changes
-------
Rebased patch with master. Incorporated Ajay Yadava's comments.
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 (updated)
-----
client/src/main/java/org/apache/falcon/cli/FalconAdminCLI.java PRE-CREATION
client/src/main/java/org/apache/falcon/cli/FalconCLI.java 1574585
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
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