Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Sqoop, mail # dev - Review Request: SQOOP-648 Moving localizable strings to resources and consolidate all String constants to Constants.java
Venkat Ranganathan
2012-12-01, 16:49
Jarek Cecho
2012-12-01, 18:49
Venkat Ranganathan
2012-12-02, 06:11
Venkat Ranganathan
2012-12-02, 06:49
Venkat Ranganathan
2012-12-03, 18:04
Jarek Cecho
2012-12-03, 18:11
Venkat Ranganathan
2012-12-05, 07:56
Jarek Cecho
2012-12-05, 16:09
Venkat Ranganathan
2012-12-05, 18:41
Jarek Cecho
2012-12-08, 15:59
Venkat Ranganathan
2012-12-08, 17:58
Jarek Cecho
2012-12-08, 18:57
Copy link to this message
-
Re: Review Request: SQOOP-648 Moving localizable strings to resources and consolidate all String constants to Constants.java
Jarek Cecho 2012-12-02, 17:07

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8305/#review13957
-----------------------------------------------------------
Hi Venkat,
thank you very much for this fast turnaround. I still do have couple of notes, I'm sorry to not putting them all together at once:
client/src/main/java/org/apache/sqoop/client/core/ClientError.java
<https://reviews.apache.org/r/8305/#comment29797>

    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.

client/src/main/java/org/apache/sqoop/client/core/Constants.java
<https://reviews.apache.org/r/8305/#comment29798>

    Constantize commands and parameters was originally outside of current JIRA scope, however it seems very beneficial. Thank you for doing that!

client/src/main/java/org/apache/sqoop/client/shell/CloneJobFunction.java
<https://reviews.apache.org/r/8305/#comment29800>

    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?

client/src/main/java/org/apache/sqoop/client/shell/CloneJobFunction.java
<https://reviews.apache.org/r/8305/#comment29799>

    I would suggest to create constants for short arguments rather than using charAt() function here.

client/src/main/java/org/apache/sqoop/client/shell/ShowCommand.java
<https://reviews.apache.org/r/8305/#comment29801>

    What about putting this code to SqoopCommand class similarly as I've proposed for SqoopFunction?

client/src/main/resources/client-resource.properties
<https://reviews.apache.org/r/8305/#comment29803>

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

client/src/main/resources/client-resource.properties
<https://reviews.apache.org/r/8305/#comment29804>

    Would you mind putting resource keys in lower case?

client/src/main/resources/client-resource.properties
<https://reviews.apache.org/r/8305/#comment29805>

    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

client/src/main/resources/client-resource.properties
<https://reviews.apache.org/r/8305/#comment29802>

    I believe that there is a typo  and "{0|" should be "{0}", right?
Thank you very much for your effort!

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