> On Oct. 20, 2014, 6:26 p.m., Jarek Cecho wrote: > > Overall I like the changes. I do have couple of high level notes: > > > > 1) Please do remove all the trailing whitespaces. I've mark some of them. > > Most of the IDEs do have option to automatically remove trailing whispaces > > on file save, so you might want to enable that option :) > > > > 2) It seems that this patch also incorporates quite a lot of changes into > > the query names and pretty much refactores the Query class. I don't mind > > doing those changes, but they should be covered by a separate JIRA as they > > are unrelated to "rename SQ_CONFIGURABLE". > > Veena Basavaraj wrote: > WS yes, I dont usually give it much thought until the end of the review, > I do have it enable din IDE, > > While I was doing the upgrade it was really hard for me to follow things, > when people just add things without much documentation. > > If you want another RB, I will spend time on it, splitting this up, but I > dont want to scarifice code readability for some superficial deadline, so I > will split it up.
Having proper JIRA per problem and not commit "uber JIRAs" is not for code redability nor to meet some deadlines. It's for us to enable tools such as "git cherry-pick" or "git blame" effectively which in the long term is must have based on my experience. > On Oct. 20, 2014, 6:26 p.m., Jarek Cecho wrote: > > common/src/main/java/org/apache/sqoop/model/MDriver.java, lines 71-73 > > <https://reviews.apache.org/r/26941/diff/1/?file=725833#file725833line71> > > > > Why returning String, wouldn't it make more sense to return the enumb > > MConfigurableType directly? > > Veena Basavaraj wrote: > this method is most used to persist the value in database. I dont like > hardcoding int he derby, like most other places have done esp cial for the > config types. > > If we have more repository implementaitons tomm, we can just use the > getType method. I'm not following why would one use of this method in Derby database yeild the need to return String? Why can't we return enum that will be subsequently converted to String by Derby itself? In my mind the model classes are very general as anyone can use them (they are in common package after all), so I'm not particularly found of the idea to design the API for one specific use case. I would assume that on calling getType(), I would get a type that I can easily compare (if statement) or put into switch statement, so I really don't see use case for String here. - Jarek ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26941/#review57377 ----------------------------------------------------------- On Oct. 20, 2014, 6:27 p.m., Veena Basavaraj wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26941/ > ----------------------------------------------------------- > > (Updated Oct. 20, 2014, 6:27 p.m.) > > > Review request for Sqoop. > > > Repository: sqoop-sqoop2 > > > Description > ------- > > see JIRA for details > > there is whitespace, that will be addressed once the reviews for the > functionality > > > Diffs > ----- > > common/src/main/java/org/apache/sqoop/model/MConfigurableType.java > PRE-CREATION > common/src/main/java/org/apache/sqoop/model/MConnector.java 2f42191 > common/src/main/java/org/apache/sqoop/model/MDriver.java 685439e > core/src/main/java/org/apache/sqoop/driver/Driver.java f1b45bb > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoError.java > 3e4a4a9 > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java > c888910 > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaConstants.java > 59773e1 > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java > 44ec2e3 > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java > 366e4ee > > Diff: https://reviews.apache.org/r/26941/diff/ > > > Testing > ------- > > yes > > > Thanks, > > Veena Basavaraj > >
