> On Feb. 4, 2015, 2:25 p.m., Jarek Cecho wrote: > > 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) > > Veena Basavaraj wrote: > it is done when w register the configs/ inserting to the database, wont > this cover all paths,?
The word defensive did not make sense to me and confused me, defensive mean we catch the issue and the current code does it, if it a matter of checking this in the config utils and not in repository code, i will move it there, not a big deal. - Veena ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30537/#review71043 ----------------------------------------------------------- On Feb. 4, 2015, 2: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, 2: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 > >
