Review Board 1.7.22


SQOOP-648 Moving localizable strings to resources and consolidate all String constants to Constants.java

Review Request #8305 - Created Dec. 1, 2012 and updated

Venkat Ranganathan
Reviewers
Sqoop
jarcec
sqoop-sqoop2
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
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
Total:
13
Open:
13
Resolved:
0
Dropped:
0
Status:
From:
Description From Last Updated Status
Sqoop is currently using common schema for exception all over the place and the named constants are quite important. I ... Jarek Cecho Dec. 1, 2012, 6:49 p.m. Open
What about using static call MessageFormat.format here and on all other places? That way we would skip explicit object and ... Jarek Cecho Dec. 2, 2012, 6:12 a.m. Open
Sqoop is currently using common schema for exception all over the place and the named constants are quite important - ... Jarek Cecho Dec. 2, 2012, 5:07 p.m. Open
I can see repetition of this code fragment in almost every file. What about putting it into SqoopFunction class and ... Jarek Cecho Dec. 2, 2012, 5:07 p.m. Open
I would suggest to create constants for short arguments rather than using charAt() function here. Jarek Cecho Dec. 2, 2012, 5:07 p.m. Open
What about putting this code to SqoopCommand class similarly as I've proposed for SqoopFunction? Jarek Cecho Dec. 2, 2012, 5:07 p.m. Open
I would suggest not to use error codes in resources for the moment as we need to introduce way to ... Jarek Cecho Dec. 2, 2012, 5:07 p.m. Open
Would you mind putting resource keys in lower case? Jarek Cecho Dec. 2, 2012, 5:07 p.m. Open
Would be great to have hierarchical path in the resources similarly as Hadoop is using. One way that would think ... Jarek Cecho Dec. 2, 2012, 5:07 p.m. Open
I believe that there is a typo and "{0|" should be "{0}", right? Jarek Cecho Dec. 2, 2012, 5:07 p.m. Open
Would you mind removing this unused import? Jarek Cecho Dec. 5, 2012, 4:09 p.m. Open
I believe that dropping the "final" keyword is not necessary here, right? Jarek Cecho Dec. 5, 2012, 4:09 p.m. Open
I would suggest to merging this two properties into one as they have exactly the same value and this will ... Jarek Cecho Dec. 5, 2012, 4:09 p.m. Open
Review request changed
Updated (Dec. 8, 2012, 5:58 p.m.)
Hi Jarcec 

Thanks for reviewing.  I have updated the client resource to add the new version string and also updated ShowConnector.java.

I have tested to make sure that the Version string is printed in show connector option
Ship it!
Posted (Dec. 8, 2012, 6:57 p.m.)
Hi Venkat,
thank you very much for your quick turn around. It's looks good to me, so please attach last version of your patch to JIRA and I'll commit it.

Jarcec