----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25148/#review51861 -----------------------------------------------------------
Ship it! Looks good to me. One comment on the testing below. common/src/test/java/org/apache/sqoop/validation/TestValidationRunner.java <https://reviews.apache.org/r/25148/#comment90498> Nice :) common/src/test/java/org/apache/sqoop/validation/validators/TestContains.java <https://reviews.apache.org/r/25148/#comment90871> Comments are a bit confusing here. This seems like duplicate logic? - Abraham Elmahrek On Aug. 28, 2014, 11:41 a.m., Jarek Cecho wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25148/ > ----------------------------------------------------------- > > (Updated Aug. 28, 2014, 11:41 a.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-1468 > https://issues.apache.org/jira/browse/SQOOP-1468 > > > Repository: sqoop-SQOOP-1367 > > > Description > ------- > > The patch is a bit bigger and more intrusive than I've anticipated due to > quite huge limitations that Java imposes on what can be used in annotations. > Nevertheless I belive that I've arrived to quite nice and user friendly > solution. The patch is actually not as big as it might seem, list of changes: > > * I've renamed Validator class to AbstractValidator (majority of the changes > is just the rename) > * Created new annotation "Validator" > * Instead of taking array of AbstractValidator classes I've changed > Class/Form/Input annotations to accept array of annotations "Validator" - > this is important as this is what allows us to specify arguments and create > validators such as "Contains" or "StartsWith" > * Created new validators "Contains" and "StartsWith" and used them in Generic > JDBC Connector > > Beauty of this solution is that adding a new parameters (like int parameter, > ...) is a backward compatible change provided that the parameter will have > default value. > > > Diffs > ----- > > common/src/main/java/org/apache/sqoop/model/ConfigurationClass.java 5323bd9 > common/src/main/java/org/apache/sqoop/model/FormClass.java 48bff3c > common/src/main/java/org/apache/sqoop/model/Input.java 61fc01a > common/src/main/java/org/apache/sqoop/model/Validator.java PRE-CREATION > common/src/main/java/org/apache/sqoop/validation/ValidationResult.java > 2782fac > common/src/main/java/org/apache/sqoop/validation/ValidationRunner.java > 46e2d56 > > common/src/main/java/org/apache/sqoop/validation/validators/AbstractValidator.java > PRE-CREATION > > common/src/main/java/org/apache/sqoop/validation/validators/ClassAvailable.java > 2adfe6c > common/src/main/java/org/apache/sqoop/validation/validators/Contains.java > PRE-CREATION > common/src/main/java/org/apache/sqoop/validation/validators/NotEmpty.java > 520beea > common/src/main/java/org/apache/sqoop/validation/validators/NotNull.java > fb8a926 > common/src/main/java/org/apache/sqoop/validation/validators/StartsWith.java > PRE-CREATION > common/src/main/java/org/apache/sqoop/validation/validators/Validator.java > bdb7c20 > common/src/test/java/org/apache/sqoop/validation/TestValidationRunner.java > 1961425 > > common/src/test/java/org/apache/sqoop/validation/validators/TestClassAvailable.java > 511b3b4 > > common/src/test/java/org/apache/sqoop/validation/validators/TestContains.java > PRE-CREATION > > common/src/test/java/org/apache/sqoop/validation/validators/TestNotEmpty.java > d225b06 > > common/src/test/java/org/apache/sqoop/validation/validators/TestNotNull.java > 9c5bed5 > > common/src/test/java/org/apache/sqoop/validation/validators/TestStartsWith.java > PRE-CREATION > > common/src/test/java/org/apache/sqoop/validation/validators/TestValidator.java > 1a5dbdd > > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/ConnectionForm.java > e513770 > > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/FromTableForm.java > 1c0b429 > > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/ToTableForm.java > 0601a39 > > Diff: https://reviews.apache.org/r/25148/diff/ > > > Testing > ------- > > Added unit tests for new functionality, existist tests are passing. > > > Thanks, > > Jarek Cecho > >
