> On Oct. 14, 2014, 4:25 a.m., Veena Basavaraj wrote: > > common/src/main/java/org/apache/sqoop/model/ConfigUtils.java, line 236 > > <https://reviews.apache.org/r/26609/diff/1/?file=719198#file719198line236> > > > > might be good to be consistent with getName > > to avoid names clashes ( different conenctors can have the same class > > names) > > > > http://stackoverflow.com/questions/15202997/what-is-the-difference-between-canonical-name-simple-name-and-class-name-in-jav
I'm not sure that I'm following your point with "getName()" on line when I'm comparing innstance for null. Are you by any chance referring to the line below when I'm trowing an exception with configClass.getSimpleName()? If so, then fixed :) > On Oct. 14, 2014, 4:25 a.m., Veena Basavaraj wrote: > > core/src/main/java/org/apache/sqoop/repository/Repository.java, line 437 > > <https://reviews.apache.org/r/26609/diff/1/?file=719201#file719201line437> > > > > please make a method that can be reused for checking the validation > > result. Could you elaborate a bit more what you mean? Such method is already there right? It's the "canProceed()". Or do you see issue with the dual method call ".getStatus().canProceed()"? > On Oct. 14, 2014, 4:25 a.m., Veena Basavaraj wrote: > > core/src/main/java/org/apache/sqoop/repository/Repository.java, line 487 > > <https://reviews.apache.org/r/26609/diff/1/?file=719201#file719201line487> > > > > please move repeated code to a class I'm sorry, it seems that the comment has been done on empty line, so I'm not sure what you're referring to. > On Oct. 14, 2014, 4:25 a.m., Veena Basavaraj wrote: > > core/src/main/java/org/apache/sqoop/repository/Repository.java, line 465 > > <https://reviews.apache.org/r/26609/diff/1/?file=719201#file719201line465> > > > > same as above. can we move this to a class, since there is the same > > duplication below. Could you elaborate a bit more what do you have on your mind? It seems that the comment has been made only on one line. > On Oct. 14, 2014, 4:25 a.m., Veena Basavaraj wrote: > > core/src/main/java/org/apache/sqoop/repository/Repository.java, line 591 > > <https://reviews.apache.org/r/26609/diff/1/?file=719201#file719201line591> > > > > nice, thanks for the join! Not sure what you're thanking for as the comment is highlighint empty line :) > On Oct. 14, 2014, 4:25 a.m., Veena Basavaraj wrote: > > core/src/test/java/org/apache/sqoop/repository/TestJdbcRepository.java, > > line 264 > > <https://reviews.apache.org/r/26609/diff/1/?file=719202#file719202line264> > > > > curious how is the testing now done? We are no longer having explicit Validator, so I've removed the explicit order call. However we are still testing the that validator runs and negative case will stop the upgrade. Check out test testDriverConfigUpgradeWithInvalidJobs(). > On Oct. 14, 2014, 4:25 a.m., Veena Basavaraj wrote: > > core/src/test/java/org/apache/sqoop/repository/TestJdbcRepository.java, > > line 835 > > <https://reviews.apache.org/r/26609/diff/1/?file=719202#file719202line835> > > > > Passing? can we rename it to Validating ..or something along those lines > > > > please use a type in the name so its easy to see which type of config > > is validated. > > > > > > > > @ConfigurationClass > > public static class EmptyLinkConfiguration { > > } > > @ConfigurationClass > > public static class EmptyJobConfiguration { > > } Classes itself don't have a type, so I'm using one class for both. Seems simpler and easier for purpose of unit testing. > On Oct. 14, 2014, 4:25 a.m., Veena Basavaraj wrote: > > server/src/main/java/org/apache/sqoop/handler/JobRequestHandler.java, line > > 203 > > <https://reviews.apache.org/r/26609/diff/1/?file=719203#file719203line203> > > > > so no more driver config validation? Not sure that I'm following this point entirely. Could you elaborate? Perhaps highlighing entire section that caught your eye would help :) - Jarek ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26609/#review56489 ----------------------------------------------------------- On Oct. 11, 2014, 9:51 p.m., Jarek Cecho wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26609/ > ----------------------------------------------------------- > > (Updated Oct. 11, 2014, 9:51 p.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-1576 > https://issues.apache.org/jira/browse/SQOOP-1576 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > I've migrated Repository validation structure to the new infra. I've noticed > some code reuse between Repository and Server Handlers, so I've refactored > those to ConfigUtils class. > > > Diffs > ----- > > common/src/main/java/org/apache/sqoop/model/ConfigUtils.java 9e762dc > common/src/main/java/org/apache/sqoop/model/ModelError.java 35a8943 > common/src/test/java/org/apache/sqoop/model/TestConfigUtils.java d5377f8 > core/src/main/java/org/apache/sqoop/repository/Repository.java 95c7a4d > core/src/test/java/org/apache/sqoop/repository/TestJdbcRepository.java > e6e4760 > server/src/main/java/org/apache/sqoop/handler/JobRequestHandler.java > 462579c > server/src/main/java/org/apache/sqoop/handler/LinkRequestHandler.java > 80e65b8 > > Diff: https://reviews.apache.org/r/26609/diff/ > > > Testing > ------- > > Unit tests seems to be passing. > > > Thanks, > > Jarek Cecho > >
