----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26948/#review63855 -----------------------------------------------------------
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 repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryHandler.java <https://reviews.apache.org/r/26948/#comment106160> Future: Perhaps a suggestion for future, I think that this can be further improved to simply use PrepareStatement.setObject() to support any type :) repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoError.java <https://reviews.apache.org/r/26948/#comment106161> Nit: Can we do this change in separate JIRA? Cleaning up Derby repository structures as part of "Introduce PostgreSQL repository" doesn't seem right :) repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java <https://reviews.apache.org/r/26948/#comment106162> Nit: Similarly for this method re-order, let's not do it as part of introducing PostgreSQL repository. repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlRepoConstants.java <https://reviews.apache.org/r/26948/#comment106163> We don't have to be on equal version with Derby - this is clean start, so let's start with 1? repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlRepoError.java <https://reviews.apache.org/r/26948/#comment106164> Super nit: Let's put comma after the final one, so that adding new error code doesn't require changing this line. repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlRepositoryHandler.java <https://reviews.apache.org/r/26948/#comment106165> 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 :) repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlRepositoryHandler.java <https://reviews.apache.org/r/26948/#comment106166> I'm assuming that those runQuery(*_CREATE_*) should be inside the if (version == 0 ) statement? Otherwise we will try to create the tables on every method execution which I assume will always fail on the cond try, right? repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlRepositoryHandler.java <https://reviews.apache.org/r/26948/#comment106167> Since we are throwing exception here, let's use LOG.error or LOG.fatal intead of LOG.warn? :) repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlRepositoryHandler.java <https://reviews.apache.org/r/26948/#comment106169> Let's change the comment to valid "PostgreSQL" Query? :) repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlSchemaCreateQuery.java <https://reviews.apache.org/r/26948/#comment106176> What about making reasonable exception in our "no asterisks imports" rule here :) repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlSchemaQuery.java <https://reviews.apache.org/r/26948/#comment106177> 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. repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/PostgreSQLTestUtils.java <https://reviews.apache.org/r/26948/#comment106178> Let's not however use asterisk import for java.util package classes. repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/PostgreSQLTestUtils.java <https://reviews.apache.org/r/26948/#comment106179> Let's perhaps call those statements "assertTableExists", it seems that that is the way we are using in other assert-like methods. repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/PostgresqlTestCase.java <https://reviews.apache.org/r/26948/#comment106181> The provider is loading the driver on the start method already: https://github.com/apache/sqoop/blob/sqoop2/common-test/src/main/java/org/apache/sqoop/common/test/db/DatabaseProvider.java#L126 repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/PostgresqlTestCase.java <https://reviews.apache.org/r/26948/#comment106190> 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 repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/PostgresqlTestCase.java <https://reviews.apache.org/r/26948/#comment106182> I'm wondering why the entire package name in the call here - it seems that we are importing other Junit related classes. Jarcec - Jarek Cecho 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 > >
