-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61372/#review182693
-----------------------------------------------------------
Hi Zach,
Thanks for your contribution, this is a really great improvement for Sqoop
Metastore usage!
In general your patch looks good and well organized, however there are some
problems with your change.
I ran unit and 3rd party tests and had JobToolTest,
MetaConnectIncrementalImportTest and SavedJobsTest failing with many
AssertionFailedErrors, for example:
FAILED
expected:<0> but was:<1>
junit.framework.AssertionFailedError: expected:<0> but was:<1>
at
com.cloudera.sqoop.metastore.JobToolTest.testDeleteJob(JobToolTest.java:257
also in SavedJobs I got these error messages for many test cases:
Exception creating SQL connection
java.io.IOException: Exception creating SQL connection
at
org.apache.sqoop.metastore.GenericJobStorage.init(GenericJobStorage.java:270)
at
org.apache.sqoop.metastore.GenericJobStorage.open(GenericJobStorage.java:208)
at
com.cloudera.sqoop.metastore.SavedJobsTest.setUp(SavedJobsTest.java:150)
Caused by: java.sql.SQLSyntaxErrorException: ORA-00942: table or view does not
exist
at oracle.jdbc.driver.T4CTTIoer.processError(T4CTTIoer.java:447)
at oracle.jdbc.driver.T4CTTIoer.processError(T4CTTIoer.java:396)
at oracle.jdbc.driver.T4C8Oall.processError(T4C8Oall.java:951)
at oracle.jdbc.driver.T4CTTIfun.receive(T4CTTIfun.java:513)
at oracle.jdbc.driver.T4CTTIfun.doRPC(T4CTTIfun.java:227)
at oracle.jdbc.driver.T4C8Oall.doOALL(T4C8Oall.java:531)
at
oracle.jdbc.driver.T4CPreparedStatement.doOall8(T4CPreparedStatement.java:208)
at
oracle.jdbc.driver.T4CPreparedStatement.executeForDescribe(T4CPreparedStatement.java:886)
at
oracle.jdbc.driver.OracleStatement.executeMaybeDescribe(OracleStatement.java:1175)
at
oracle.jdbc.driver.OracleStatement.doExecuteWithTimeout(OracleStatement.java:1296)
at
oracle.jdbc.driver.OraclePreparedStatement.executeInternal(OraclePreparedStatement.java:3613)
at
oracle.jdbc.driver.OraclePreparedStatement.executeQuery(OraclePreparedStatement.java:3657)
at
oracle.jdbc.driver.OraclePreparedStatementWrapper.executeQuery(OraclePreparedStatementWrapper.java:1495)
at
org.apache.sqoop.metastore.GenericJobStorage.getRootProperty(GenericJobStorage.java:620)
at
org.apache.sqoop.metastore.GenericJobStorage.init(GenericJobStorage.java:236)
or
ERROR: must be owner of relation sqoop_root
org.postgresql.util.PSQLException: ERROR: must be owner of relation sqoop_root
at
org.postgresql.core.v3.QueryExecutorImpl.receiveErrorResponse(QueryExecutorImpl.java:2157)
at
org.postgresql.core.v3.QueryExecutorImpl.processResults(QueryExecutorImpl.java:1886)
at
org.postgresql.core.v3.QueryExecutorImpl.execute(QueryExecutorImpl.java:255)
at
org.postgresql.jdbc2.AbstractJdbc2Statement.execute(AbstractJdbc2Statement.java:555)
at
org.postgresql.jdbc2.AbstractJdbc2Statement.executeWithFlags(AbstractJdbc2Statement.java:403)
at
org.postgresql.jdbc2.AbstractJdbc2Statement.execute(AbstractJdbc2Statement.java:395)
at
com.cloudera.sqoop.metastore.SavedJobsTest.resetSchema(SavedJobsTest.java:184)
at
com.cloudera.sqoop.metastore.SavedJobsTest.resetJobSchema(SavedJobsTest.java:167)
at
com.cloudera.sqoop.metastore.SavedJobsTest.setUp(SavedJobsTest.java:140)
Caused an ERROR
null
java.lang.NullPointerException
at
com.cloudera.sqoop.metastore.SavedJobsTest.tearDown(SavedJobsTest.java:156)
Additonaly, could you please add some short guidelines to the new tests at the
beginning of the files about how to run them? Please take a look at the
SQLServer 3rd party test cases for examples.
I would also prefer to have some error messages at the Assertion checks as
currently those are not so verbose and doesn't really help understanding the
failures.
Thanks,
Bogi
src/test/com/cloudera/sqoop/metastore/SavedJobsTest.java
Lines 68 (patched)
<https://reviews.apache.org/r/61372/#comment258677>
This test class could be split into smaller ones as currently it runs 54
test cases which is not so effective for example for debugging purposes. Please
group tests into separate test classes based on functionality they are testing.
- Boglarka Egyed
On Aug. 9, 2017, 8:53 a.m., Zach Berkowitz wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61372/
> -----------------------------------------------------------
>
> (Updated Aug. 9, 2017, 8:53 a.m.)
>
>
> Review request for Sqoop, Boglarka Egyed, Jarek Cecho, and Anna Szonyi.
>
>
> Bugs: SQOOP-3216
> https://issues.apache.org/jira/browse/SQOOP-3216
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> Expanded Metastore support for MySql, Oracle, Postgresql, MSSql, and DB2
>
>
> Diffs
> -----
>
> src/docs/user/metastore-purpose.txt e7eb23d3
> src/docs/user/saved-jobs.txt e8757801
> src/java/com/cloudera/sqoop/metastore/hsqldb/AutoHsqldbStorage.java
> 259d9f63
> src/java/com/cloudera/sqoop/metastore/hsqldb/HsqldbJobStorage.java 083e2a37
> src/java/org/apache/sqoop/SqoopOptions.java 2eb3d8a4
> src/java/org/apache/sqoop/manager/CubridManager.java 5a1a0e83
> src/java/org/apache/sqoop/manager/Db2Manager.java 61b6868d
> src/java/org/apache/sqoop/manager/HsqldbManager.java 9b9c5822
> src/java/org/apache/sqoop/manager/JdbcDrivers.java PRE-CREATION
> src/java/org/apache/sqoop/manager/MySQLManager.java 3c2276fe
> src/java/org/apache/sqoop/manager/NetezzaManager.java 0ac77175
> src/java/org/apache/sqoop/manager/OracleManager.java 2f4585cc
> src/java/org/apache/sqoop/manager/PostgresqlManager.java 44e041ad
> src/java/org/apache/sqoop/manager/SQLServerManager.java 9a3d9183
> src/java/org/apache/sqoop/manager/SupportedManagers.java 8a6037af
> src/java/org/apache/sqoop/metastore/JobStorageFactory.java 2edc33b8
> src/java/org/apache/sqoop/metastore/hsqldb/AutoHsqldbStorage.java 49e30319
> src/java/org/apache/sqoop/metastore/hsqldb/HsqldbJobStorage.java a0f29fd0
> src/java/org/apache/sqoop/tool/BaseSqoopTool.java 1564bdcb
> src/java/org/apache/sqoop/tool/JobTool.java 054e274f
> src/test/com/cloudera/sqoop/TestIncrementalImport.java 52a55b78
> src/test/com/cloudera/sqoop/metastore/JobToolTest.java PRE-CREATION
> src/test/com/cloudera/sqoop/metastore/MetaConnectIncrementalImportTest.java
> PRE-CREATION
> src/test/com/cloudera/sqoop/metastore/SavedJobsTest.java PRE-CREATION
> src/test/com/cloudera/sqoop/metastore/TestSavedJobs.java 61d8c97d
> src/test/findbugsExcludeFile.xml a27ec378
>
>
> Diff: https://reviews.apache.org/r/61372/diff/1/
>
>
> Testing
> -------
>
> Three test classes SavedJobsTest, MetaConnectIncrementalImportTest, and
> JobToolTest all pass for all supported databases.
>
>
> Thanks,
>
> Zach Berkowitz
>
>
