> On Dec. 4, 2014, 6:30 p.m., Jarek Cecho wrote: > > Good job Abe! I've just read the code and I'm still yet to try the changes > > on real cluster, but I wanted to share my thoughts early. I have couple of > > high level questions and then couple of nits: > > > > 1) Are you also planning to provide functional tests in addition to the > > structural ones? Like create link/job and verifying other calls that are > > prescribed by Repository interface [1] > > > > 2) Are you planning to provide a documentation on how to use the new > > repository? Perhaps a section in the Administration guide talking about > > different repository implementations? > > > > 3) The tests are creating a lot of tables, but I did not see any clean up. > > In case that I've did not simply missed it, we should add it, otherwise > > test re-execution won't be that simple. > > > > Links: > > 1: > > https://github.com/apache/sqoop/blob/sqoop2/core/src/main/java/org/apache/sqoop/repository/Repository.java > > Jarek Cecho wrote: > Please scratch 3), I found the clean up routine :) > > Jarek Cecho wrote: > I've give it a spin on my real cluster and I can confirm that it's > working just fine, thank you Abe!
Functional tests are in https://issues.apache.org/jira/browse/SQOOP-1591. Documentation is dependent on https://issues.apache.org/jira/browse/SQOOP-1680. Can create follow up on this though. Tests are removing entire schema in PostgresqlTestCase#tearDown. Is there any other clean up that you think might be useful? > On Dec. 4, 2014, 6:30 p.m., Jarek Cecho wrote: > > repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlRepositoryHandler.java, > > lines 83-84 > > <https://reviews.apache.org/r/26948/diff/3/?file=779836#file779836line83> > > > > If we want to use the constant in Lower case, then let's define them as > > such? It seems weird to have the contant in upper case and then lowercase > > it on every use :) > > Jarek Cecho wrote: > Actually, I would generalize my comment. When playing with the PostgreSQL > repository on real cluster, I've noticed that PostgreSQL will lower case all > table/column names that are not explicilty quoted. Hence I would suggest to > either lower case all the constants or escape them in the queries - whatever > we prefer more :) In that case, for consistency, would you feel ok with having every thing lower case (table names, column names, etc.)? > On Dec. 4, 2014, 6:30 p.m., Jarek Cecho wrote: > > repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlSchemaQuery.java, > > lines 23-26 > > <https://reviews.apache.org/r/26948/diff/3/?file=779839#file779839line23> > > > > I see the same very long comment in file PostgresqlSchemaCreateQuery. > > Let's perhaps have it in one single file and reference it in the other? I'm > > afraid that having it in several places will be harder for maintenance. https://issues.apache.org/jira/browse/SQOOP-1824 > On Dec. 4, 2014, 6:30 p.m., Jarek Cecho wrote: > > repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/PostgresqlTestCase.java, > > lines 44-48 > > <https://reviews.apache.org/r/26948/diff/3/?file=779841#file779841line44> > > > > I'm a bit worried about this approach as it will hide configuration > > issues - e.g. if the database is down or credentials are missconfigured, > > the test will still pass and the fact that we are not testing PostgreSQL > > repo will be silently ignored. > > > > Let's perhaps promote those tests into Integration tests similarly as > > we're doing in test module [1] and add a flag to enable them? > > > > Another idea would be to use JUnit categories [2] and it's maven > > integration to run them in a special profile/flag. > > > > I'm happy to take this one into subsequent JIRA as it's more general > > topic though. > > > > Links: > > 1: https://github.com/apache/sqoop/blob/sqoop2/test/pom.xml#L154 > > 2: > > https://weblogs.java.net/blog/johnsmart/archive/2010/04/25/grouping-tests-using-junit-categories-0 > > 3: > > http://maven.apache.org/surefire/maven-surefire-plugin/examples/junit.html Interesting ideas! I've created SQOOP-1591 to address testing of this repository. > On Dec. 4, 2014, 6:30 p.m., Jarek Cecho wrote: > > repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryHandler.java, > > line 2203 > > <https://reviews.apache.org/r/26948/diff/3/?file=779830#file779830line2203> > > > > Future: Perhaps a suggestion for future, I think that this can be > > further improved to simply use PrepareStatement.setObject() to support any > > type :) Cool. - Abraham ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26948/#review63855 ----------------------------------------------------------- On Dec. 2, 2014, 1:48 a.m., Abraham Elmahrek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26948/ > ----------------------------------------------------------- > > (Updated Dec. 2, 2014, 1:48 a.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-1523 > https://issues.apache.org/jira/browse/SQOOP-1523 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > commit 0702bddfa69f63f56bfc6909dfe1b3e4f3219c6a > Author: Abraham Elmahrek <[email protected]> > Date: Tue Oct 14 12:05:53 2014 -0700 > > Sqoop2: Postgresql repository > > :100644 100644 5a8e026... 2c5c972... M > core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java > :100644 100644 f25a29f... 5be6ad9... M pom.xml > :100644 100644 e3345c4... 8b308c9... M repository/pom.xml > :000000 100644 0000000... 19f67d4... A > repository/repository-postgresql/pom.xml > :000000 100644 0000000... cdeed35... A > repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlRepoConstants.java > :000000 100644 0000000... 872683e... A > repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlRepoError.java > :000000 100644 0000000... 93352a6... A > repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlRepositoryHandler.java > :000000 100644 0000000... d1ff898... A > repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlSchemaConstants.java > :000000 100644 0000000... d70dd86... A > repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlSchemaQuery.java > :000000 100644 0000000... 31cc6dc... A > repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/PostgresqlTestCase.java > :000000 100644 0000000... a8f4242... A > repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/TestConnectorHandling.java > :000000 100644 0000000... 44ffced... A > repository/repository-postgresql/src/test/resources/log4j.properties > :100644 100644 67baaa5... 8ecdd01... M server/pom.xml > > > Diffs > ----- > > common-test/pom.xml 9fd671c > > common-test/src/main/java/org/apache/sqoop/common/test/db/DatabaseProvider.java > 0a12d7b > > common-test/src/main/java/org/apache/sqoop/common/test/db/PostgreSQLProvider.java > d46e01d > core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java > 4fe1500 > pom.xml e6ffc78 > repository/pom.xml 8c95c0e > > repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryHandler.java > c278406 > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoError.java > 6bc5674 > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java > 907978f > repository/repository-postgresql/pom.xml PRE-CREATION > > repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlRepoConstants.java > PRE-CREATION > > repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlRepoError.java > PRE-CREATION > > repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlRepositoryHandler.java > PRE-CREATION > > repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlSchemaConstants.java > PRE-CREATION > > repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlSchemaCreateQuery.java > PRE-CREATION > > repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlSchemaQuery.java > PRE-CREATION > > repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/PostgreSQLTestUtils.java > PRE-CREATION > > repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/PostgresqlTestCase.java > PRE-CREATION > > repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/PostgresqlTestUtils.java > PRE-CREATION > > repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/TestStructure.java > PRE-CREATION > repository/repository-postgresql/src/test/resources/log4j.properties > PRE-CREATION > server/pom.xml 1adcca0 > > Diff: https://reviews.apache.org/r/26948/diff/ > > > Testing > ------- > > Can start Sqoop2 server. > Ran integration tests with PostgreSQL. All pass except partitioning on a > boolean column. > Also: > 482 disable link -l 1 > 483 show link --all > 484 show link > 485 enable link -l 1 > 486 show link > 487 show link --all > 488 show job > 489 show job --all > 490 show connector > 491 show connector --all > 492 help > 493 status job --jid 1 > 494 history > 495 start job --jid 1 > 496 clone job --jid 1 > 497 delete job --jid 2 > 498 status job --jid 1 > 499 history > > > Thanks, > > Abraham Elmahrek > >
