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

Switch to Plain 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
+
Venkat Ranganathan 2012-12-02, 06:11
+
Venkat Ranganathan 2012-12-02, 06:49
+
Jarek Cecho 2012-12-02, 17:07
+
Jarek Cecho 2012-12-03, 18:11
Copy link to this message
-
Re: Review Request: SQOOP-648 Moving localizable strings to resources and consolidate all String constants to Constants.java


> 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:
+
Venkat Ranganathan 2012-12-05, 07:56
+
Venkat Ranganathan 2012-12-05, 18:41
+
Venkat Ranganathan 2012-12-08, 17:58
+
Jarek Cecho 2012-12-08, 18:57
+
Jarek Cecho 2012-12-08, 15:59
+
Jarek Cecho 2012-12-05, 16:09
+
Jarek Cecho 2012-12-01, 18:49