> On Oct. 20, 2014, 11:26 a.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. > > Jarek Cecho wrote: > 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.
sure have created a ticket for documenting the derby schema code, migh as well split it up into classes for create / upgrade and the CRUD operations since you are open to it now. > On Oct. 20, 2014, 11:26 a.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. > > Jarek Cecho wrote: > 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. Sure. I think it makes sense to have Enum. changed. - Veena ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26941/#review57377 ----------------------------------------------------------- On Oct. 20, 2014, 11:27 a.m., Veena Basavaraj wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26941/ > ----------------------------------------------------------- > > (Updated Oct. 20, 2014, 11:27 a.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 > >
