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