----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30537/#review71043 -----------------------------------------------------------
Thank you very much for putting this patch together Veena! Overall I like the approach and have only few questions/comments (mostly real nits): 1) I think that I broke the patch by committing postgresql integration tests: [ERROR] /Users/jarcec/apache/sqoop2-vanila/repository/repository-postgresql/src/test/java/org/apache/sqoop/integration/repository/postgresql/PostgresqlTestCase.java:[151,19] cannot find symbol [ERROR] symbol : constructor MStringInput(java.lang.String,boolean,short) [ERROR] location: class org.apache.sqoop.model.MStringInput [ERROR] /Users/jarcec/apache/sqoop2-vanila/repository/repository-postgresql/src/test/java/org/apache/sqoop/integration/repository/postgresql/PostgresqlTestCase.java:[153,12] cannot find symbol 2) I'm slightly worries that we are not doing any defensive checking for the overrides field. We are keeping the "overrides" only as comma separated string and we are not verifying anywhere that it actually contains only valid values (e.g. other configs that do exists). The only place where we are sort of doing that is in the repository where we are actually translating the values to ids, but that seems quite late in the process. Do you think that we could add those checks somewhere sooner? Perhaps to the ConfigUtils class? (e.g. let's fail fast) common/src/main/java/org/apache/sqoop/error/code/CommonRepositoryError.java <https://reviews.apache.org/r/30537/#comment116568> Super nit: can we add comma at this line? By doing this, we won't have to change this line next time we will be adding a new error code which makes use of "git blame" slightly easier. common/src/main/java/org/apache/sqoop/model/Input.java <https://reviews.apache.org/r/30537/#comment116569> Nit: I know that we had discussion about making it array of objects and decided not to because it would be too complex, but what perhaps array of strings? Like: String [] overrides() default {}; We have similar idiom with the validators (it's an array of something), so it seems a bit nicer. I don't feel super strongly about this one though, so if we feel that this is better, I'm fine. repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryHandler.java <https://reviews.apache.org/r/30537/#comment116572> I'm not immediately sure why we are removing this fragment? repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaCreateQuery.java <https://reviews.apache.org/r/30537/#comment116586> It seems to me that we are storing the same information twice - we have the input names stored in one table and their corresponding ids in second table. I'm wondering if that is necessary? repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaCreateQuery.java <https://reviews.apache.org/r/30537/#comment116581> Can we put somewhere (probably into ConfigUtils) verification code that will ensure that the string is less then the 32 characters? I'm afraid that connector developper will try to use more and "random" things will start happening. repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaCreateQuery.java <https://reviews.apache.org/r/30537/#comment116584> I think that the id here is not adding much value, right? The primary key can be simply (PARENT_ID, CHILD_ID). repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaCreateQuery.java <https://reviews.apache.org/r/30537/#comment116575> Nit: Trailing whitespace repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java <https://reviews.apache.org/r/30537/#comment116576> Nit: It seems taht we are just removing the spaces which we seem to be using everywhere else in the project? (applicable for entire file) Jarcec - Jarek Cecho On Feb. 4, 2015, 10:02 p.m., Veena Basavaraj wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30537/ > ----------------------------------------------------------- > > (Updated Feb. 4, 2015, 10:02 p.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-1804 > https://issues.apache.org/jira/browse/SQOOP-1804 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > see JIRA > > derby updates > for input attributes > and storing input relations > > > Diffs > ----- > > common/src/main/java/org/apache/sqoop/error/code/CommonRepositoryError.java > 952be3f > common/src/main/java/org/apache/sqoop/json/util/ConfigInputConstants.java > 5d261de > > common/src/main/java/org/apache/sqoop/json/util/ConfigInputSerialization.java > 8b11ed5 > common/src/main/java/org/apache/sqoop/model/ConfigUtils.java 279f3a6 > common/src/main/java/org/apache/sqoop/model/Input.java b6305f2 > common/src/main/java/org/apache/sqoop/model/InputEditable.java PRE-CREATION > common/src/main/java/org/apache/sqoop/model/MBooleanInput.java c7fcf90 > common/src/main/java/org/apache/sqoop/model/MConfig.java d128441 > common/src/main/java/org/apache/sqoop/model/MEnumInput.java 74838fc > common/src/main/java/org/apache/sqoop/model/MInput.java d1c2ab6 > common/src/main/java/org/apache/sqoop/model/MIntegerInput.java de50cd4 > common/src/main/java/org/apache/sqoop/model/MMapInput.java cf781b2 > common/src/main/java/org/apache/sqoop/model/MStringInput.java 4854838 > common/src/test/java/org/apache/sqoop/json/util/ConfigTestUtil.java 7f0e2f1 > > common/src/test/java/org/apache/sqoop/json/util/TestConfigSerialization.java > 10ac3cf > common/src/test/java/org/apache/sqoop/model/TestConfigUtils.java 64ffdd1 > common/src/test/java/org/apache/sqoop/model/TestMAccountableEntity.java > c0644e7 > common/src/test/java/org/apache/sqoop/model/TestMBooleanInput.java 57d2da2 > common/src/test/java/org/apache/sqoop/model/TestMConfig.java 908348d > common/src/test/java/org/apache/sqoop/model/TestMConfigList.java 4f8261c > common/src/test/java/org/apache/sqoop/model/TestMConnector.java e1c02aa > common/src/test/java/org/apache/sqoop/model/TestMEnumInput.java 39f09ce > common/src/test/java/org/apache/sqoop/model/TestMIntegerInput.java 69e511f > common/src/test/java/org/apache/sqoop/model/TestMJob.java 605f429 > common/src/test/java/org/apache/sqoop/model/TestMLink.java e985c17 > common/src/test/java/org/apache/sqoop/model/TestMMapInput.java c9b786b > common/src/test/java/org/apache/sqoop/model/TestMNamedElement.java f851cbd > common/src/test/java/org/apache/sqoop/model/TestMStringInput.java a4faf95 > common/src/test/java/org/apache/sqoop/model/TestMValidatedElement.java > f0bdda4 > > connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestGenericJdbcConnectorUpgrader.java > 1b1fa7f > core/src/test/java/org/apache/sqoop/driver/TestDriverConfigUpgrader.java > d4522ae > > repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryHandler.java > 4feaee6 > > repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryInsertUpdateDeleteSelectQuery.java > 8626b31 > > repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositorySchemaConstants.java > 4ab07b2 > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java > 2f05fcb > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaConstants.java > a551094 > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaCreateQuery.java > ad80797 > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaUpgradeQuery.java > fa6710b > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java > be8c23e > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestConnectorHandling.java > ca24398 > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestDriverHandling.java > e12bf46 > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestInputTypes.java > fb07152 > > repository/repository-postgresql/src/test/java/org/apache/sqoop/integration/repository/postgresql/PostgresqlTestCase.java > 08a3342 > shell/src/main/java/org/apache/sqoop/shell/core/Constants.java ca387d8 > shell/src/main/java/org/apache/sqoop/shell/utils/ConfigDisplayer.java > e240163 > shell/src/main/java/org/apache/sqoop/shell/utils/ConfigFiller.java de40818 > shell/src/main/resources/shell-resource.properties 2b5a9b7 > > Diff: https://reviews.apache.org/r/30537/diff/ > > > Testing > ------- > > yes > > > Thanks, > > Veena Basavaraj > >
