sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Venkat Ranganathan" <n....@live.com>
Subject Re: Review Request: SQOOP-648 Moving localizable strings to resources and consolidate all String constants to Constants.java
Date Mon, 03 Dec 2012 18:04:36 GMT


> On Dec. 2, 2012, 5:07 p.m., Jarek Cecho wrote:
> > client/src/main/resources/client-resource.properties, line 61
> > <https://reviews.apache.org/r/8305/diff/3/?file=232829#file232829line61>
> >
> >     I believe that there is a typo  and "{0|" should be "{0}", right?

Thanks.  I did not realize this issue.  Will check again


> On Dec. 2, 2012, 5:07 p.m., Jarek Cecho wrote:
> > client/src/main/resources/client-resource.properties, lines 45-47
> > <https://reviews.apache.org/r/8305/diff/3/?file=232829#file232829line45>
> >
> >     Would be great to have hierarchical path in the resources similarly as Hadoop
is using. One way that would think about it would be to use command/function for making the
hierarchy, for example:
> >     
> >     clone.conn_successful = Connection ...
> >     clone.usage = Usage: clone

Did not check hadoop - will do so


> On Dec. 2, 2012, 5:07 p.m., Jarek Cecho wrote:
> > client/src/main/resources/client-resource.properties, line 39
> > <https://reviews.apache.org/r/8305/diff/3/?file=232829#file232829line39>
> >
> >     Would you mind putting resource keys in lower case?

Will do


> On Dec. 2, 2012, 5:07 p.m., Jarek Cecho wrote:
> > client/src/main/resources/client-resource.properties, lines 25-34
> > <https://reviews.apache.org/r/8305/diff/3/?file=232829#file232829line25>
> >
> >     I would suggest not to use error codes in resources for the moment as we need
to introduce way to how translate all exceptions from entire Sqoop first (including server
for example).

Sure, currently these are no longer used, but will remove them from resources also


> On Dec. 2, 2012, 5:07 p.m., Jarek Cecho wrote:
> > client/src/main/java/org/apache/sqoop/client/shell/ShowCommand.java, lines 37-38
> > <https://reviews.apache.org/r/8305/diff/3/?file=232811#file232811line37>
> >
> >     What about putting this code to SqoopCommand class similarly as I've proposed
for SqoopFunction?

Sure.  Good thing to refactor


> On Dec. 2, 2012, 5:07 p.m., Jarek Cecho wrote:
> > client/src/main/java/org/apache/sqoop/client/shell/CloneJobFunction.java, line 60
> > <https://reviews.apache.org/r/8305/diff/3/?file=232800#file232800line60>
> >
> >     I would suggest to create constants for short arguments rather than using charAt()
function here.

Good point.  It looks kind of odd in a way


> On Dec. 2, 2012, 5:07 p.m., Jarek Cecho wrote:
> > client/src/main/java/org/apache/sqoop/client/shell/CloneJobFunction.java, lines
49-50
> > <https://reviews.apache.org/r/8305/diff/3/?file=232800#file232800line49>
> >
> >     I can see repetition of this code fragment in almost every file. What about
putting it into SqoopFunction class and provide public getResource() method that can be used
in all subclasses?

Good point - can be refactored.


> On Dec. 2, 2012, 5:07 p.m., Jarek Cecho wrote:
> > client/src/main/java/org/apache/sqoop/client/core/Constants.java, lines 31-35
> > <https://reviews.apache.org/r/8305/diff/3/?file=232796#file232796line31>
> >
> >     Constantize commands and parameters was originally outside of current JIRA scope,
however it seems very beneficial. Thank you for doing that!

Thanks you


> On Dec. 2, 2012, 5:07 p.m., Jarek Cecho wrote:
> > client/src/main/java/org/apache/sqoop/client/core/ClientError.java, lines 22-45
> > <https://reviews.apache.org/r/8305/diff/3/?file=232795#file232795line22>
> >
> >     Sqoop is currently using common schema for exception all over the place and
the named constants are quite important - including the message text. I would suggest to leave
the exception handling "as it is" for now and file additional JIRA for translating them. E.g.
let's put into resources everything but exceptions.

Thanks Jarek.  Will change them including the constants.   But I am not sure I understand
about the common schema.  Just trying to understand better.  Is there a table with all the
exceptions persisted?


On Dec. 2, 2012, 5:07 p.m., Venkat Ranganathan wrote:
> > Thank you very much for your effort!

Thanks a lot for taking the time to review.   Will upload after making the changes and testing
them


- Venkat


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


On Dec. 2, 2012, 6:49 a.m., Venkat Ranganathan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8305/
> -----------------------------------------------------------
> 
> (Updated Dec. 2, 2012, 6:49 a.m.)
> 
> 
> Review request for Sqoop and Jarek Cecho.
> 
> 
> Description
> -------
> 
> I have moved localizable strings to the client resources (those that are descriptions,
messages in general etc).  Also consolidated constants to one place and removed repetitive
occurrences.   
> 
> 4 more files in utils need to be updated, but wanted to get this reviewed and take that
after this
> 
> 
> Diffs
> -----
> 
>   client/src/main/java/org/apache/sqoop/client/core/ClientError.java fd3b97d 
>   client/src/main/java/org/apache/sqoop/client/core/Constants.java 47c0547 
>   client/src/main/java/org/apache/sqoop/client/request/Request.java 1720507 
>   client/src/main/java/org/apache/sqoop/client/shell/CloneCommand.java 847a6ad 
>   client/src/main/java/org/apache/sqoop/client/shell/CloneConnectionFunction.java 21c41aa

>   client/src/main/java/org/apache/sqoop/client/shell/CloneJobFunction.java b0e8d90 
>   client/src/main/java/org/apache/sqoop/client/shell/CreateCommand.java 2453543 
>   client/src/main/java/org/apache/sqoop/client/shell/CreateConnectionFunction.java 734276d

>   client/src/main/java/org/apache/sqoop/client/shell/CreateJobFunction.java 0b685bf 
>   client/src/main/java/org/apache/sqoop/client/shell/DeleteCommand.java bb09bf3 
>   client/src/main/java/org/apache/sqoop/client/shell/DeleteConnectionFunction.java ee2a1cf

>   client/src/main/java/org/apache/sqoop/client/shell/DeleteJobFunction.java acc8e21 
>   client/src/main/java/org/apache/sqoop/client/shell/HelpCommand.java 03499d8 
>   client/src/main/java/org/apache/sqoop/client/shell/SetCommand.java 20c8090 
>   client/src/main/java/org/apache/sqoop/client/shell/SetOptionFunction.java 3764306 
>   client/src/main/java/org/apache/sqoop/client/shell/SetServerFunction.java daf1ff4 
>   client/src/main/java/org/apache/sqoop/client/shell/ShowCommand.java bd74253 
>   client/src/main/java/org/apache/sqoop/client/shell/ShowConnectionFunction.java 4e49288

>   client/src/main/java/org/apache/sqoop/client/shell/ShowConnectorFunction.java a34c48c

>   client/src/main/java/org/apache/sqoop/client/shell/ShowFrameworkFunction.java 8dcf976

>   client/src/main/java/org/apache/sqoop/client/shell/ShowJobFunction.java ee8c63d 
>   client/src/main/java/org/apache/sqoop/client/shell/ShowServerFunction.java 039e28b

>   client/src/main/java/org/apache/sqoop/client/shell/ShowVersionFunction.java 9e8c607

>   client/src/main/java/org/apache/sqoop/client/shell/SqoopCommand.java 9ae693e 
>   client/src/main/java/org/apache/sqoop/client/shell/SqoopFunction.java 200b3ee 
>   client/src/main/java/org/apache/sqoop/client/shell/SqoopShell.java b2d05f4 
>   client/src/main/java/org/apache/sqoop/client/shell/SubmissionCommand.java 39a2b31 
>   client/src/main/java/org/apache/sqoop/client/shell/SubmissionStartFunction.java 74ce905

>   client/src/main/java/org/apache/sqoop/client/shell/SubmissionStatusFunction.java 4d683c0

>   client/src/main/java/org/apache/sqoop/client/shell/SubmissionStopFunction.java 97628f7

>   client/src/main/java/org/apache/sqoop/client/shell/UpdateCommand.java 5bac209 
>   client/src/main/java/org/apache/sqoop/client/shell/UpdateConnectionFunction.java 4e55dba

>   client/src/main/java/org/apache/sqoop/client/shell/UpdateJobFunction.java f7cdf26 
>   client/src/main/java/org/apache/sqoop/client/utils/ThrowableDisplayer.java 45c78fb

>   client/src/main/resources/client-resource.properties 201efe9 
> 
> Diff: https://reviews.apache.org/r/8305/diff/
> 
> 
> Testing
> -------
> 
> Ran the SQOOP2 client tests and manually ran various client commands to make sure that
all commands have their localizable strings and constants properly displayed apart from running
all the unit tests.   No new tests were added
> 
> 
> Thanks,
> 
> Venkat Ranganathan
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message