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