Mailing-List: contact dev-help@falcon.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@falcon.apache.org
Content-Type: multipart/alternative;
 boundary="===============1731975395093260586=="
MIME-Version: 1.0
Subject: Re: Review Request 40481: Need ability to provide the path for recipe
 files in command line
From: "Peeyush Bishnoi" <bpeeyush@yahoo.co.in>
To: "Peeyush Bishnoi" <bpeeyush@yahoo.co.in>,
 "Falcon" <dev@falcon.incubator.apache.org>,
 "Ajay Yadava" <ajaynsit@gmail.com>
Date: Mon, 23 Nov 2015 08:41:31 -0000
Message-ID: <20151123084131.26797.77295@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Peeyush Bishnoi" <noreply@reviews.apache.org>
References: <20151120024018.19610.59691@reviews.apache.org>
In-Reply-To: <20151120024018.19610.59691@reviews.apache.org>
Reply-To: "Peeyush Bishnoi" <bpeeyush@yahoo.co.in>

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


> On Nov. 20, 2015, 2:40 a.m., Ajay Yadava wrote:
> >

Thanks Ajay for reviewing.


> On Nov. 20, 2015, 2:40 a.m., Ajay Yadava wrote:
> > client/src/main/java/org/apache/falcon/cli/FalconRecipeCLI.java, line 107
> > <https://reviews.apache.org/r/40481/diff/1/?file=1131969#file1131969line107>
> >
> >     I think only having a file exists check should be enough for validation.

This check I have put up, to ensure that recipe properties file name must match to recipe name as per the recipe current design.


> On Nov. 20, 2015, 2:40 a.m., Ajay Yadava wrote:
> > client/src/main/java/org/apache/falcon/cli/FalconRecipeCLI.java, line 43
> > <https://reviews.apache.org/r/40481/diff/1/?file=1131969#file1131969line43>
> >
> >     Do you think "file" instead of "properties" will be more intuitive?

"file" is more general but here for recipe we want to specifically provide recipe properties file that's why "properties" looks better.


> On Nov. 20, 2015, 2:40 a.m., Ajay Yadava wrote:
> > client/src/main/java/org/apache/falcon/cli/FalconRecipeCLI.java, line 110
> > <https://reviews.apache.org/r/40481/diff/1/?file=1131969#file1131969line110>
> >
> >     It is a mandatory parameter and if it is blank then it's fine?

recipePropertiesFile is mandatory parameter while passing through properties argument on command line. Value to option "-properties" can't be null . If recipe properties file is not passed, then properties file must be available inside "falcon.recipe.path" as usual.


> On Nov. 20, 2015, 2:40 a.m., Ajay Yadava wrote:
> > client/src/main/java/org/apache/falcon/cli/FalconRecipeCLI.java, line 119
> > <https://reviews.apache.org/r/40481/diff/1/?file=1131969#file1131969line119>
> >
> >     Any reason for having the restriction that the properties file should match the recipe name? I am not able to appreciate it's usefulness and it looks restrictive to me.

As per current design of recipe, it looks for template and properties file with recipe name only.

String recipeFilePath = recipePath + File.separator + recipeName + TEMPLATE_SUFFIX;
String propertiesFilePath = recipePath + File.separator + recipeName + PROPERTIES_SUFFIX;

And through this patch, I don't want to change much behaviour, that's why I aligned with current implementation of recipe.
Hoping such restriction will not be available when recipe server side will be implemented.


> On Nov. 20, 2015, 2:40 a.m., Ajay Yadava wrote:
> > client/src/main/java/org/apache/falcon/client/FalconClient.java, line 1108
> > <https://reviews.apache.org/r/40481/diff/1/?file=1131970#file1131970line1108>
> >
> >     Can you please add it in the client.properties so that it's easier for users to figure out which property value to override?

done.


> On Nov. 20, 2015, 2:40 a.m., Ajay Yadava wrote:
> > client/src/main/java/org/apache/falcon/client/FalconClient.java, line 1152
> > <https://reviews.apache.org/r/40481/diff/1/?file=1131970#file1131970line1152>
> >
> >     Earlier this path was the value of ""falcon.recipe.path" in client.properties and now it has extra sub path added to it. Was it broken earlier?

No. Now templateFilePath is also from the value of "falcon.recipe.path" only. 
With this patch, if properties file is passed on command line then we will use properfiles file directory path to look for recipe template and workflow file. This obtained directory path from properties file will become "falcon.recipe.path".


- Peeyush


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


On Nov. 19, 2015, 2:02 p.m., Peeyush Bishnoi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40481/
> -----------------------------------------------------------
> 
> (Updated Nov. 19, 2015, 2:02 p.m.)
> 
> 
> Review request for Falcon.
> 
> 
> Bugs: FALCON-1588
>     https://issues.apache.org/jira/browse/FALCON-1588
> 
> 
> Repository: falcon-git
> 
> 
> Description
> -------
> 
> FALCON-1588 : Need ability to provide the path for recipe files in command line
> 
> 
> Diffs
> -----
> 
>   client/src/main/java/org/apache/falcon/cli/FalconRecipeCLI.java b93fed2 
>   client/src/main/java/org/apache/falcon/client/FalconClient.java c49dd08 
> 
> Diff: https://reviews.apache.org/r/40481/diff/
> 
> 
> Testing
> -------
> 
> yes.
> 
> 
> Thanks,
> 
> Peeyush Bishnoi
> 
>


--===============1731975395093260586==--